Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[HLSL] add loop unroll #93879
[HLSL] add loop unroll #93879
Changes from all commits
87fe5df
2e0dd44
b4415b2
7bd21a6
3eba000
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in your comment, CheckLoopHintExpr will check each of these asserts, and return true in each case, so nullptr will be returned by this function on line 608. That said, is there really a purpose for these asserts? I don't see how they can trigger when CheckLoopHintExpr confirms these conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. You are correct it is not needed, I added it as a form of future proofing. Let me explain my thinking.
CheckLoopHintExpr
is the sema checks initially created for clang loop unrolling. We are just leveraging it. There are typically two ways of enforcing a contract on expected behavior, unit tests and asserts. I typically like to include both asserts and unit tests. of the two asserts are my preference because they encode your expectations in the code base and are not tangential to it like unit tests.As a thought experiment lets imagine a change to
CheckLoopHintExpr
. This is unlikely to happen, but for the sake of argument lets say the behavior ofCheckLoopHintExpr
changes to one day support non constant ints. Then the nullptr would not return and we would get a fall through. We want alerts to this regression in behavior on the HLSL portion and asserts are my prefered way of knowing what happend for the reasons mentioned above but also because you get a callstack. I use to write a bunch of c# code, we had code contracts to define expectations. Thats kind of what i'm trying to do here.