Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review #4

Open
wants to merge 21 commits into
base: empty
Choose a base branch
from
Open

Review #4

wants to merge 21 commits into from

Conversation

1whatleytay
Copy link
Owner

No description provided.

Copy link
Owner Author

@1whatleytay 1whatleytay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review from @pent0.

Notes: Const where possible. C++ compiler certainly can optimize things better.

src/gxp/src/builder.cpp Outdated Show resolved Hide resolved
src/gxp/include/gxp/usse.h Outdated Show resolved Hide resolved
src/gxp/include/gxp/builder.h Outdated Show resolved Hide resolved
src/gxp/src/usse.cpp Show resolved Hide resolved
}
}

std::string getVaryingName(ProgramVarying varying) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::string getVaryingName(ProgramVarying varying) {

  • const char * will be better. You are constructing heap memory... What if you want to use const char * but not string somewhere in future.
  • Try to stick with built-in type whenever you can.

src/translator/include/translator/translator.h Outdated Show resolved Hide resolved
bool isStruct();
};

class CompilerGXP : public Compiler {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The design is in a way unfriendly with future optimization like another layer of IR.
  • Another layer of IR maybe needed because the USSE ops have many opcodes that can optimize SPIRV to the very end.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'm not really aware of any other methods though. I started reading the dragon book a few days ago, it might have a better idea?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even have it, so idk... Go with your instics.. Personally I try and error and figure out what is better, than copy code from somewhere and make it better... Though this optimization is hard to do. We can keep this note for now.

@1whatleytay
Copy link
Owner Author

Updated to latest :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants