[InferAddressSpaces] Add InferAddressSpaces pass to pipeline for SPIR #7418
Conversation
Clang generates 'addrspacecast' instructions to align address spaces between alloca/global variables/kernel parameters and flat address space references (i.e. addrspace(4)). For the SPIR/SPIR-V target, addrspace(4) is the generic address space and these addrspacecast instructions can be safely removed from the code. To perform this removing, the InferAddressSpaces pass has been added to the clang optimization pipeline for SPIR and SPIR-V targets. This pass should be run after the other optimization passes (both function and module) and, it is very important, after inlining to let the pass "understand" from which address space as many as possible variables came and eliminate as many as possible addrspacecast instructions. The elimination of redundant addrspacecast instruction decreases the size of the generated SPIR-V module and therefore makes less pressure on the backend JIT compilers.
|
/summary:run |
|
This is re-creation of #5905, which was closed with no reviews. |
|
|
||
| #pragma once | ||
|
|
||
| namespace clang { |
There was a problem hiding this comment.
I'm not sure if this is the right place for this. Can this information be added to /clang/lib/Basic/Targets/SPIR.h
There was a problem hiding this comment.
+1
IMO, a better approach would be to expose a flat address space via TargetInfo which pulls the actual value from the actual target.
But I think the rational approach is to add a getFlatAddressSpace override to llvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h, this would make the infer pass pull the flat address space value from the TTI. The issue is this will force DPC++ to build the SPIR-V backend, it will have no impact on a normal sycl compilation flow but could result in users in invoking the wrong path if they tweak things.
There was a problem hiding this comment.
I'm not sure if this is the right place for this. Can this information be added to /clang/lib/Basic/Targets/SPIR.h
This information is already there. Unfortunately, we SPIR.h header is located in lib directory, so it's not available to lib/CodeGen sources.
But I think the rational approach is to add a
getFlatAddressSpaceoverride tollvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h, this would make the infer pass pull the flat address space value from the TTI. The issue is this will force DPC++ to build the SPIR-V backend, it will have no impact on a normal sycl compilation flow but could result in users in invoking the wrong path if they tweak things.
Thats right. I would like to use this approach, but today we target spir triple, which has no TTI. Once SPIR-V backend is matured enough to support SYCL, we can remove the duplication and use llvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h.
This code was written original author of the patch. See this commit - 7c28d7d. I can revert this commit to avoid adding a new header and use named constant in CodeGen lib sources. My assumption is this header is added to include with assumption that this information will be needed by other clang libraries. Let me know which option you prefer.
There was a problem hiding this comment.
This information is already there. Unfortunately, we SPIR.h header is located in lib directory, so it's not available to lib/CodeGen sources.
I think you can access TargetInfo via Context in CodeGen. If you add an interface (similar to getConstantAddressSpace()) which you ovverride for SPIR target wouldn't it work? If not, can you please explain why ?
There was a problem hiding this comment.
I think EmitAssemblyHelper has no access to ASTContext or TargetInfo.
There was a problem hiding this comment.
Right, my point was is there something stopping us from passing CodeGenModule/TargetInfo. Actually I just saw that data layout string is accessible where EmitAssemblyHelper is called. Is the information you need accessible via DataLayout string?
I'm not saying we need to do this by the way. I'm just trying to understand why we are not going this route if it is possible to do so. I found it odd that we need to add a 'Targets' folder in Basic when no other Target seems to require this. But I haven't worked as much with this side of the code so my thoughts may be incorrect. I don't want to hold up the code if @premanandrao @Naghasan and @smanna12 find current solution acceptable.
There was a problem hiding this comment.
I think
EmitAssemblyHelperhas no access toASTContextorTargetInfo.
It doesn't as far as I can see. I see that as a work around anyway, I think might just be better to have that in a file name that doesn't clash with the rest. You could simply have it in EmitAssemblyHelper.
Is the information you need accessible via DataLayout string?
I checked, didn't see any reference.
I found it odd that we need to add a 'Targets' folder in Basic when no other Target seems to require this.
Other targets have this value pulled from the backend via the TargetTransformInfo (which is what I suggested to use), for instance the definition for AMDGCN.
I don't want to hold up the code
Same, I just have a reservation on the file name but still fine as is.
but today we target spir triple, which has no TTI. Once SPIR-V backend is matured enough to support SYCL, we can remove the duplication and use
I'm not suggesting to use the SPIR-V backend as a mean to produce the SPIR-V IR. And the driver won't do that anyway. It is just a case to build the SPIR-V backend so we can make use of target info, features etc.
SPIR could be added as one of the SPIR-V target (from dpc++, this is just a triple really). I know upstream has reservations, but as there is no other target that uses this triple, I don't really see the harm as SPIR is supposed to be used in conjunction with -emit-llvm (again, I'm not suggesting to use the backend to produce anything, just pull data and control generic transformations). We can also emit a warning / error if someone tries to emit SPIR-V using the backend with the SPIR triple if we are worried ppl start to use it as a "normal path".
I have other use cases where this would be extremely useful to do this but I'll stop here as I'm diverging way too much, probably best for an offline discussion.
There was a problem hiding this comment.
SPIR could be added as one of the SPIR-V target (from dpc++, this is just a triple really). I know upstream has reservations, but as there is no other target that uses this triple, I don't really see the harm as SPIR is supposed to be used in conjunction with
-emit-llvm(again, I'm not suggesting to use the backend to produce anything, just pull data and control generic transformations).
Do you have any pointers to implementations of this approach? I don't think I've seen anything like this in the upstream.
There was a problem hiding this comment.
I've created a separate issue - #7670 to continue discussion there as this PR is merged.
|
sort.cpp issue handled at intel/llvm-test-suite#1396 |
Clang generates 'addrspacecast' instructions to align address spaces between alloca/global variables/kernel parameters and flat address space pointers (i.e. addrspace(4)).
For the SPIR/SPIR-V target, addrspace(4) is the generic address space and these addrspacecast instructions can be safely removed from the code when named address space can be deduced.
To perform this removing, the InferAddressSpaces pass has been added to the clang optimization pipeline for SPIR and SPIR-V targets. This pass should be run after the other optimization passes (both function and module) and, it is very important, after inlining to let the pass "understand" from which address space as many as possible variables came and eliminate as many as possible addrspacecast instructions.
The elimination of redundant addrspacecast instruction decreases the size of the generated SPIR-V module and therefore makes less pressure on the backend/JIT compilers.