-
Notifications
You must be signed in to change notification settings - Fork 281
check constexpr of the nested calls #6820
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
base: master
Are you sure you want to change the base?
Conversation
6df1194
to
b8bf2d6
Compare
/format |
🌈 Formatted, please merge the changes from this PR |
break; | ||
|
||
callee = returnVal; | ||
changedInThisInst = checkInstConstExprRecursively(context, ii); |
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.
I think this should be changedInThisInst |= checkInstConstExprRecursively(context, ii);
, otherwise changedInThisInst
may reset back to false if a later instruction didn't change.
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.
The recursion going through this line causes a stack overflow if a recursive function is given in input, like in tests/diagnostics/recursion.slang
.
|
||
if (maybeMarkConstExprBackwardPass(context, arg)) | ||
{ | ||
changedInThisInst = true; |
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.
I'm wondering for all of those changedInThisInst = true;
can we just early return? It doesn't look like we need to go through everything.
This change aims to report error when the nested func requires constexpr, and the caller calls wrapper func with non-cost params.
For example, the following should failed to compile because RWTexture2D::Load requires offset to be constexpr.
Fixes: #6370