-
Notifications
You must be signed in to change notification settings - Fork 100
Fix fragment input deductions in max_variables_count,input
#4546
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: main
Are you sure you want to change the base?
Fix fragment input deductions in max_variables_count,input
#4546
Conversation
| { numVariablesDelta: 0, useExtraBuiltinInputs: true }, | ||
| { numVariablesDelta: -1, useExtraBuiltinInputs: true }, | ||
| { numVariablesDelta: -3, useExtraBuiltinInputs: 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.
nit: Test -2 and -3 because those are the cases on either side of the boundary. (Helps catch off-by-one errors or if an implementation tries to merge two of the variables but not the third.)
| { numVariablesDelta: 0, useExtraBuiltinInputs: true }, | |
| { numVariablesDelta: -1, useExtraBuiltinInputs: true }, | |
| { numVariablesDelta: -3, useExtraBuiltinInputs: true }, | |
| { numVariablesDelta: -2, useExtraBuiltinInputs: true }, | |
| { numVariablesDelta: -3, useExtraBuiltinInputs: 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.
Added the -2 case with the latest force-push to 7c3fdf7. I want to clarify, though: your suggestion block would strip the -1 and 0 cases, and I'm guessing that's not intentional. Is that right?
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.
IIUC (please let me know if I'm misunderstanding the test):
It used to test -1 and 0 because those were on either side of the boundary (-1 would give space for the builtins, 0 would not). Now the boundary is between -3 (max valid) and -2 (min invalid). I think it should be pretty safe to assume that if the implementation gets -2 correct, then it will get -1 and 0 correct as well.
To be fair this is not clear because of the structure of the test since it computes success in the test body instead of passing it along with each case. It would probably be a good idea to do a little refactor here and make success a parameter (_success: true / _success: false) so we can actually tell that the test is testing the boundaries it intends to.
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.
Makes sense. Changed cases (but not the _success refactor) as suggested with latest force-push to f9fd2cc.
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.
Ah, but wait: If this is compat, then -1 is still an interesting boundary case to test, isn't it? It seems like that's still interesting to keep. That also means that we can't naively add _success to the case parameters, because whether it should succeed or not would actually depend on how many built-ins are being added, which is variable.
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.
Force-pushed to e0157af to include -1.
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.
very good point... I should have actually looked at the code again before suggesting things.
I pulled it locally and experimented with another fix idea. Couldn't push to your branch so I made a new PR here that includes your commits and mine. PTAL: #4555
98ec99c to
7c3fdf7
Compare
|
Results for build job (at e0157af): -webgpu:api,validation,render_pipeline,inter_stage:max_variables_count,input:* - 8 cases, 8 subcases (~1/case)
+webgpu:api,validation,render_pipeline,inter_stage:max_variables_count,input:* - 10 cases, 10 subcases (~1/case)
-TOTAL: 284495 cases, 2341917 subcases
+TOTAL: 284497 cases, 2341919 subcases |
faf6b48 to
f9fd2cc
Compare
f9fd2cc to
e0157af
Compare
Issue: #4538
See also #4539.
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.