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
Add test plan for oneapi_non_uniform_groups extension #866
Add test plan for oneapi_non_uniform_groups extension #866
Changes from 2 commits
3cac56c
a29b71d
48afb56
ac9a6f4
b514678
2fb29ef
284624d
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.
It confused me, but this is what the specification says.
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.
Ironically, the implementation and extension specification slightly disagree on this. I have a patch to make them agree: intel/llvm#12905
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.
Are you excluding the case of 1. That seems interesting to test corner cases, otherwise.
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.
For the class API tests we only test for one group configuration. The intention here was to pick one that wasn't 1 to avoid some trivial cases and to test a more normal group size. We could potentially run the tests for multiple configurations, if that would be more interesting? We do it for the group algorithms.
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.
Oh I thought you were running with several N and in that case testing also the boundaries would have made sense.
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 it's important for the CTS to deliberately check usage of a tangle in two sides of a branch:
Compilers are incredibly likely to see code like this and try to be clever by removing the branch, but that ignores the desired
tangle_group
semantics. If we don't have a test like this, I don't think we can be confident that an implementation actually understands tangles.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.
Currently my WIP tests use something like:
Do you think it would be fine to separate them, i.e. something like:
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.
No, I don't think this is enough. The behavior we want to test isn't whether tangles can exist for both
condition
and!condition
, but rather that they can exist simultaneously. If the tests are split out, there's no risk of a compiler optimizing too aggressively.It might be clearer if I complete my example. The concern is that a compiler may see this:
and replace it with this:
That transformation is legal for most code, but is illegal for code containing tangle groups.
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 am of two minds regarding this. On one hand, I agree that an optimizer might elect to make that optimization, but on the other hand I don't think the CTS should make tests based on potential optimizations. An implementation could simply turn off optimizations in the CTS and pass and there could be a plethora of other patterns that could also be used to cause mayhem here, so it seems a little contrived.
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 can see where you're coming from, but I think it's important for the CTS to test things that might break.
If an implementation has to turn off optimizations to make tangle groups properly (and pass the CTS) then they will need to communicate to users that certain functionality only works with optimizations turned off. Conversely, if the CTS doesn't test tricky cases like these, implementations that don't actually support the feature will be able to claim that they do -- and if a developer complains that the implementation's behavior is wrong, the implementation can point to the CTS as proof that it isn't!
We (DPC++) needed to run a test like this in order to find out that the initial implementation wasn't working properly. So, I think there's a good chance that having a test like this might also be useful to other implementers.
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 see what you mean. Would it be enough to do it just for the API testing? I.e. all the algorithm tests can still just use one control flow? I would assume in general having the two paths in one test should be good enough to weed out the worst cases of this kind of optimizations.
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.
Yes, I think that's sufficient. That will give us some confidence that the groups are being constructed correctly in weird corner cases. We're already not going to test all possible code they can put in those branches, so I'm okay if we skip the algorithms too!
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.
Alright! I have tried specifying this in the test plan.