-
Notifications
You must be signed in to change notification settings - Fork 768
[SYCL] fix incorrect application of binary AND in tests #16610
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
[SYCL] fix incorrect application of binary AND in tests #16610
Conversation
…ng binary and will mask errors, leading the test to incorrectly pass when it should fail. I wasted an hour last week being mislead by this problem. I audited all our tests and fortunately the ones making this mistake were relatively few.
@@ -66,7 +66,7 @@ int main() { | |||
// CHECK:{{[0-9]+}}|Release buffer|[[USERID3]]|[[BEID3]] | |||
// CHECK:{{[0-9]+}}|Destruct buffer|[[USERID3]] | |||
for (int i = 0; i < 3; i++) | |||
MismatchFound &= func(Queue); | |||
MismatchFound |= func(Queue); |
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.
IMO, a better fix is to use +=
if it returns number of mismatches found, or change the function to return true/false
for success/failure.
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 didn't mean to change that to an int, have restored it back to it's original boolean declaration.
I'm disinclined to make any further change. The variable is a boolean named MismatchFound
. It's pretty clear what it is. If I were change it to be something that could default to true, it'd have to be MismatchNotFound
and then I'd also have to change the func(...)
that is called to similarly reverse its interpretation.
It's fine having a boolean initialized to false, it just means the test has to use |=
instead of &=
.
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 only approving this because it eliminates the immediate buggy behavior.
IMO, the PR should be addressing this on a slightly bigger scope by changing the return types to bool
and renaming all the int Result
to bool Success
to eliminate any ambiguity.
I also don't think that refusal to address that and relying on other contributors to address that technical debt is an exemplary behavior.
…clearer to the reader
CI failure on pvc: |
@cperkinsintel sycl/test-e2e/Basic/fpga_tests/fpga_pipes.cpp (which was changed in this PR) failed in post-commit, could you please take a look? FYI There is an old github issue, but I believe that failure hasn't been seen for a while #14308 |
&= with 0 is always 0. For tests that are returning 0 as success, using binary AND will mask errors, leading the test to incorrectly pass when it should fail. I wasted an hour last week being mislead by this problem. I audited all our tests and fortunately the ones making this mistake were relatively few.