[SYCL][ESIMD] Add function to get reference to underlying data #8725
Conversation
|
/summary:run |
|
Feedback from @v-klochkov
I'm doing this now. Edit: Implemented now |
|
@intel/dpcpp-cfe-reviewers Hi all, can you please review the CFE change? Basically we want to restrict the usage of a function to inline assembly. Let me know if there's a better way to do it. Thanks! |
clang/lib/Sema/SemaSYCL.cpp
Outdated
| @@ -11,6 +11,7 @@ | |||
| #include "TreeTransform.h" | |||
| #include "clang/AST/AST.h" | |||
| #include "clang/AST/Mangle.h" | |||
| #include "clang/AST/ParentMapContext.h" | |||
There was a problem hiding this comment.
Why was this required?
There was a problem hiding this comment.
Oh I guess this is required for the loop on L595
| @@ -11799,6 +11799,8 @@ def err_sycl_device_function_is_called_from_esimd : Error< | |||
| "SYCL device function cannot be called from an ESIMD context">; | |||
| def err_sycl_esimd_vectorize_unsupported_value : Error< | |||
| "%0 attribute argument must be 8, 16, or 32">; | |||
| def err_sycl_esimd_invalid_use_outline_inlineasm : Error< | |||
There was a problem hiding this comment.
What happens if we don't diagnose in the FE i.e. what happens when function is incorrectly called now (without FE code in this PR)?
There was a problem hiding this comment.
responded to this below but tldr nothing happens and it works fine
clang/lib/Sema/SemaSYCL.cpp
Outdated
| @@ -11,6 +11,7 @@ | |||
| #include "TreeTransform.h" | |||
| #include "clang/AST/AST.h" | |||
| #include "clang/AST/Mangle.h" | |||
| #include "clang/AST/ParentMapContext.h" | |||
There was a problem hiding this comment.
Oh I guess this is required for the loop on L595
clang/lib/Sema/SemaSYCL.cpp
Outdated
| SemaRef.getASTContext().getRecordType(Method->getParent()); | ||
| if (isSyclType(Ty, SYCLTypeAttr::esimd_simd_or_simd_mask) && | ||
| Method->getNameAsString() == "data_ref") { | ||
| for (auto Use : SemaRef.getASTContext().getParents(*e)) { |
There was a problem hiding this comment.
It looks like this will do a full traversal of the AST, which we really don't want to do. If we need this to be diagnosed in the FE, we should investigate doing this via deferred diagnostics.
There was a problem hiding this comment.
Glad I had you review this to catch that, thanks!
Yes, i think we need to do this in FE. Once we get to the middle end, the function call is always inlined and we can't reliably detect the bad case anymore.. Can you point me to an example or test that uses deferred diagnostics, I'm not familiar with them.
There was a problem hiding this comment.
For deferred diagnostics in the source code, start with SYCLDiagIfDeviceCode(...) in SemaSYCL.cpp and you can trace what we do there.
For a test that uses it, see e.g. SemaSYCL/sycl-restrict.cpp.
There was a problem hiding this comment.
thanks, i think i see what to do. ill update this pr once i have something
There was a problem hiding this comment.
nevermind, im stuck again. i can't find anywhere where we have the expr of the use of the function call when doing deferred diagnostics, which is what need here since we only want to allow this function call for a particular use (directly inside an asm stmt). did i miss something?
There was a problem hiding this comment.
@premanandrao @elizabethandrews any ideas on the above? thanks!
|
@sarnex - I think the tests from intel/llvm-test-suite#1675 needs to be moved to this PR (sycl/test-e2e). |
|
@v-klochkov ill move the tests, thanks |
|
@sarnex - unfortunately, asm_glb.cpp test needs some attention as it failed in CI. |
yep, looking into it, thanks |
This is required for inline assembly. Signed-off-by: Sarnie, Nick <[email protected]>
|
@v-klochkov i looked into automatically inserting the vstore, but 1) the solution was super flaky and relied on very specific ir patterns and 2) fundamentally didn't work with opaque pointers, so i just went with your |
This is required for inline assembly.