[SYCL] Refactor address space handling in CodeGen library #2864
Conversation
There are a lot of customizations in CodeGen library enabling generic address space as default for SYCL mode. Based on feedback from the community there is a better way to get similar results. CodeGen TargetInfo object is be used to set default address space for global variables and stack allocations.
This fixes clang/test/CodeGenSYCL/const-wg-init.cpp
bader
added
the
upstream
|
Check-clang results on my local machine: Failed Tests (24): Most of the tests fail due to invalidated FileCheck checks, but the are a few compiler crashes:
In addition to check-sycl, I see compiler crash on check-sycl suite:
|
|
/summary:run |
@bader, the difference seems to be that before, we created: Whereas, now we create: This is after the call to: |
|
This looks like a bug. I think the address space for the type must be set like this: llvm/clang/lib/CodeGen/CodeGenModule.cpp Lines 4064 to 4065 in 20cb0da |
|
Actually the fix might require calling Anyway SYCL allows casts from "named" address spaces (i.e. So casting from |
For this failure, this change in CodeGenModule.cpp fixes it:
I am not sure how to fix this failure. Before your change, this is what CGDecl.cpp:~1645 show after: After your change, this is what is shows: |
Thanks. I applied this change.
Right. This is expected. With this change |
There was a problem hiding this comment.
Please add the test case from #3005 (https://github.com/intel/llvm/blob/979f66c7c79dd1f56ecaa74188e5f156a75c8125/clang/test/CodeGenSYCL/local_var_big_init_as.cpp) to this PR
|
@bader, could you also please run https://github.com/intel/llvm-test-suite/tree/intel/SYCL/ESIMD - vector BE is very sensitive to AS changes. |
Clang creates a global variable to initialize arrays with 16+ elements. This change fixes address space for such global variables, which might be target dependent.
bader
requested review from
DenisBakhvalov and
elizabethandrews
as code owners
| return AddrSpace; | ||
|
|
||
| if (CGM.isTypeConstant(D->getType(), false)) { | ||
| if (auto ConstAS = CGM.getTarget().getConstantAddressSpace()) |
There was a problem hiding this comment.
Can you spell out auto?
There was a problem hiding this comment.
This is a copy-paste from AMDGPUTargetCodeGenInfo::getGlobalVarAddressSpace.
All the places where getConstantAddressSpace is called, authors use auto: CodeGenModule::getStringLiteralAddressSpace, castStringLiteralToDefaultAddressSpace and createReferenceTemporary.
I'm okay to apply this comment, but I think its should be applied to all instances. Right?
If so, I can create an NFC patch and merge it before - https://reviews.llvm.org/D89909. Does it sound okay to you?
There was a problem hiding this comment.
Your call -- I called it out because the type wasn't obvious from the context and on line 9996 we're doing LangAS AddrSpace = D->getType().getAddressSpace(); with the type spelled out, so I figured we might as well spell it out. However, given how trivial the use is on the next line, it's not critical.
There was a problem hiding this comment.
I'll ask John McCall (CodeGen code owner) in https://reviews.llvm.org/D89909 about this.
There was a problem hiding this comment.
A little be of context for reviewers.
As noted in the description this PR reverts existing approach for handling AS mismatches in CodeGen, which brought a couple of whitespace changes breaking code formatting guidelines, but it's because llorg code violates them.
In addition to that, one of the patches was uploaded by @againull in a separate PR - #3120. I hope his patch will go first and all changes in llvm/* directory will go away from this patch. Alternative solution would be close #3120 and commit it in scope of this PR. @againull, which way do you prefer?
| return AddrSpace; | ||
|
|
||
| if (CGM.isTypeConstant(D->getType(), false)) { | ||
| if (auto ConstAS = CGM.getTarget().getConstantAddressSpace()) |
There was a problem hiding this comment.
This is a copy-paste from AMDGPUTargetCodeGenInfo::getGlobalVarAddressSpace.
All the places where getConstantAddressSpace is called, authors use auto: CodeGenModule::getStringLiteralAddressSpace, castStringLiteralToDefaultAddressSpace and createReferenceTemporary.
I'm okay to apply this comment, but I think its should be applied to all instances. Right?
If so, I can create an NFC patch and merge it before - https://reviews.llvm.org/D89909. Does it sound okay to you?
@bader , I have merged PR#3120. |
|
/summary:run |
|
@elizabethandrews, @premanandrao, could you take a look at this patch, please?
|
Also, @elizabethandrews is traveling/on vacation. |
|
Thanks! |
|
So I see that this causes a crash of a lit test. It looks like the cast you added in CodeGenModule is invalid. |
|
@erichkeane, could you provide more details on how to reproduce this issue, please? I don't see this failure locally and all CI checks pass. |
it is happening with a check-clang on a clean version of scyl HEAD using a debug compiler. |
If all CI checks are passing... are we doing a release test rather than a release + asserts test? |
|
We are doing Release and Release + Asserts. |
// REQUIRES: arm-registered-targetThat's the reason it's not caught by CI (and my local testing). |
|
Fix - #3206. |
Checks for Clang::CodeGenSYCL/intel-fpga-reg.cpp test were autogenerated to speed up fixing of LIT tests failures after a patch "[SYCL] Refactor address space handling in CodeGen library (#2864)". Reorganize test code and tidy up checks to improve readability. Signed-off-by: Mikhail Lychkov <[email protected]>
This patch replaced SYCL specific patches aiming to implement address space handling with the approach proposed in the code review comments for https://reviews.llvm.org/D89909.
New approach: overload a couple of methods in SPIRTargetCodeGenInfo to set opencl_global address space for global variables and opencl_private for variables on the stack. The rest methods of the CodeGen library should already use these hooks and address space map to handle any inconsistencies in address spaces.