[SYCL] Extend the SpecConstants/vector-convolution-demo.cpp test #830
Conversation
ghost
marked this pull request as draft
ghost
mentioned this pull request
The test is extended with a specialization constant built upon an aligned structure. The CFE generates a padding represented as '[size x i8] undef' for such structures and the sycl-post-link tool crashed during property set generation for the specialization constant. The bug has been fixed in intel/llvm#5538, this extension is the E2E test for the fix to ensure that the default values and offsets given to the runtime are correct. Signed-off-by: Pavel Samolysov <[email protected]>
| constexpr specialization_id<coeff_t> coeff_id; | ||
|
|
||
| constexpr specialization_id<coeff_struct_t> coeff_struct_id; | ||
|
|
||
| constexpr specialization_id<coeff_struct_aligned_t> coeff_struct_aligned_id; |
There was a problem hiding this comment.
It may make the test look a little odd, but would it make sense to have two of these? In that case we are sure that one will appear before another spec constant and we can be sure that the calculated offset isn't affected by the padding.
There was a problem hiding this comment.
@steffenlarsen Thank you for the comment. I'm sorry, maybe I've got your comment wrong. Should I add another spec constant with the same type as specialization_id<coeff_struct_aligned_t>, or just remove the constexpr specialization_id<coeff_struct_t> coeff_struct_id one because it has no padding and if the constant with padding has been processed well, the constant without padding should also be processed?
There was a problem hiding this comment.
My worry is that the changes to sycl-post-link could make it difficult for the runtime to correctly calculate the sizes and offsets of the specialization constants with padding. If the padded specialization constant is the last specialization constant we would likely not see this as there is no following specialization constant and as such the amount of padding does not matter to the runtime. Adding an extra specialization constant with specialization_id<coeff_struct_aligned_t> would ensure at least one of them isn't the last specialization constant, right?
There was a problem hiding this comment.
@steffenlarsen Thank you very much for the explanation, it's crystal clear now. I've added a new specialization constant after coeff_struct_aligned_id but I see locally the test failed (in fact, it failed even with one specialization constant containing padding). The test works!
An extra specialization constant is required to check whether the runtime correctly calculates the sizes and offsets of the specialization constants with padding. Signed-off-by: Pavel Samolysov <[email protected]>
Signed-off-by: Pavel Samolysov <[email protected]>
ghost
marked this pull request as ready for review
|
@mlychkov @steffenlarsen @AlexeySachkov The fix has been merged into intel/llvm, all the tests pass. Could you review again? |
ghost
self-requested a review
…el#830) The test is extended with a specialization constant built upon an aligned structure. The CFE generates a padding represented as '[size x i8] undef' for such structures and the sycl-post-link tool crashed during property set generation for the specialization constant. The bug has been fixed in intel/llvm#5538, this extension is the E2E test for the fix to ensure that the default values and offsets given to the runtime are correct. Signed-off-by: Pavel Samolysov <[email protected]>
…el#830) The test is extended with a specialization constant built upon an aligned structure. The CFE generates a padding represented as '[size x i8] undef' for such structures and the sycl-post-link tool crashed during property set generation for the specialization constant. The bug has been fixed in intel/llvm#5538, this extension is the E2E test for the fix to ensure that the default values and offsets given to the runtime are correct. Signed-off-by: Pavel Samolysov <[email protected]>
…el/llvm-test-suite#830) The test is extended with a specialization constant built upon an aligned structure. The CFE generates a padding represented as '[size x i8] undef' for such structures and the sycl-post-link tool crashed during property set generation for the specialization constant. The bug has been fixed in intel#5538, this extension is the E2E test for the fix to ensure that the default values and offsets given to the runtime are correct. Signed-off-by: Pavel Samolysov <[email protected]>
The test is extended with a specialization constant built upon an aligned
structure. The CFE generates a padding represented as '[size x i8] undef' for
such structures and the sycl-post-link tool crashed during property set
generation for the specialization constant. The bug has been fixed in
intel/llvm#5538, this extension is the E2E test for the fix to ensure that
the default values and offsets given to the runtime are correct.
Signed-off-by: Pavel Samolysov [email protected]