[SYCL] [FPGA] Disable swapping dimension values of WorkGroupAttr arguments for semantic checks #2962
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or semantic checks This is a followup comments on intel#2906 (comment) and intel#2906 (comment). Currently For a SYCLDevice WorkGroupAttr arguments are reversed. if (S.getLangOpts().SYCLIsDevice) std::swap(XDimExpr, ZDimExpr); There are few problems with this approach (swapping order in AST): 1. A user who writes the order as X, Y, Z in their source will get Z, Y, X when they pretty print the AST. 2. The same will happen when dumping the AST to text or JSON or generating diagnostic messages since the fist and third arguments of WorkGroupAttr are reversed. 3. This is very confusing to see the different order than the oroginal source codes. max_work_group_size() follows same rules as reqd_work_group_size() in Sema and LLVM IR generation. This patch keeps the arguments in the same order as they were parsed in doing semantic checks, and when performing code generation, we would change the argument order when lowering to LLVM IR and not when creating the semantic attribute. Signed-off-by: Soumi Manna <soumi.manna@intel.com>
|
could you please review @Fznamznon? Thanks |
Fznamznon
previously approved these changes
Dec 29, 2020
There was a problem hiding this comment.
It would be great to hear from @AaronBallman here as well, but for me it looks fine.
Thanks Mariya. |
AaronBallman
previously approved these changes
Dec 29, 2020
| @@ -94,13 +94,13 @@ int main() { | |||
| h.single_task<class test_kernel5>(TRIFuncObjGood2()); | |||
| // CHECK-LABEL: FunctionDecl {{.*}}test_kernel5 | |||
| // CHECK: ReqdWorkGroupSizeAttr {{.*}} | |||
| // // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} | |||
| // // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} | |||
There was a problem hiding this comment.
You can remove the duplicate // comment token. Same below.
There was a problem hiding this comment.
Done. Sorry for the duplicate comments. Thanks @AaronBallman.
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
d4581c3
Fznamznon
approved these changes
Dec 29, 2020
|
Thank you everyone for the reviews. |
elizabethandrews
approved these changes
Dec 29, 2020
MrSidims
approved these changes
Dec 29, 2020
smanna12
added a commit
to smanna12/llvm
that referenced
this pull request
Feb 1, 2021
…work_group_size attributes
This patch
1. fixes typo
2. removes redundant comments (we have disabled swapping dimension values of WorkGroupAttr
arguments for semantic checks on intel#2962)
3. Hoist the logic from the second if into the above if (AttrValue == 0)
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
jsji
pushed a commit
that referenced
this pull request
Jan 26, 2025
Update for llvm-project commit abba01a ("[ADT] Deprecate PointerUnion::{is,get} (NFC) (#122623)", 2025-01-13). Original commit: KhronosGroup/SPIRV-LLVM-Translator@c8e8867efd44034
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a followup comments on #2906 (comment)
and #2906 (comment).
Currently for a SYCLDevice, first and third argument of WorkGroupAttr
are reversed in doing semantic checks as wells as code generation.
There are few problems with this approach (swapping order in AST):
they pretty print the AST.
generating diagnostic messages since the fist and third arguments of
WorkGroupAttr are reversed.
source codes for Semantic checks.
max_work_group_size() follows same rules as reqd_work_group_size() in Sema and LLVM IR generation.
This patch keeps the arguments in the same order as they were parsed in doing semantic checks,
and when performing code generation, we would change the argument order when lowering to
LLVM IR and not when creating the semantic attribute.
Signed-off-by: Soumi Manna soumi.manna@intel.com