Port Local Accessor and Global Offset passes to the new PM #5987
Conversation
jchlanda
force-pushed
the
jakub/new_pm_port
branch
from
83416fe to
516adb9
Compare
jchlanda
force-pushed
the
jakub/new_pm_port
branch
from
516adb9 to
8b58ffb
Compare
jchlanda
changed the title
jchlanda
force-pushed
the
jakub/new_pm_port
branch
from
8b58ffb to
610f41d
Compare
|
@bader, feel free to review as well. |
| ArrayType::get(Type::getInt32Ty(M.getContext()), 3); | ||
| ImplicitOffsetPtrType = | ||
| Type::getInt32Ty(M.getContext())->getPointerTo(TargetAS); | ||
| assert((!ImplicitOffsetIntrinsic || |
There was a problem hiding this comment.
How is this possible? Don't we early return at line 79?
There was a problem hiding this comment.
Yeap, you are right, will simplify.
Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>
Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>
|
@aelovikov-intel I think I've addressed all your comments, please give me a shout if I've missed anything. |
|
ping @asudarsa |
Looking at this review now. Thanks |
| class ModulePass; | ||
| class PassRegistry; | ||
|
|
||
| /// This pass operates on SYCL kernels. It looks for uses of the |
There was a problem hiding this comment.
Can this comment be: This pass operates on SYCL kernels that target AMDGPU or NVVM.
|
|
||
| private: | ||
| /// After the execution of this function, the module to which the kernel | ||
| /// `Func` belongs contains a clone of the original kernel with the signature |
There was a problem hiding this comment.
Can you please specify if the original kernel is deleted or not? Thanks
There was a problem hiding this comment.
It is not, will expand the comment.
|
|
||
| } // end anonymous namespace | ||
| DenseMap<Function *, MDNode *> GlobalOffsetPass::validateKernels( |
There was a problem hiding this comment.
This function does two tasks: 1. validate kernels and 2. extract all entry point metadata. For readability sake, it could be useful be split it into two separate functions, one for each of the above tasks. Thanks
There was a problem hiding this comment.
I agree that the naming might be a bit misleading, how about I rename it to generateKernelMDNodeMap and update the comments. Splitting the function to do essentially one operation over the same loop of kernels seems a bit wasteful.
| virtual llvm::StringRef getPassName() const override { | ||
| return "SYCL Local Accessor to Shared Memory"; | ||
| bool runOnModule(Module &M) override { | ||
| ModuleAnalysisManager DummyMAM; |
There was a problem hiding this comment.
Nit: This can just be called MAM
This patch ports Local Accessor and Global Offset passes to the new pass manager. As most of other passes in both AMDGPU and NVPTX backends are still running with the legacy PM, it provides a legacy struct that wraps around the new PM and lets the old interface be used. Fixes: intel#5310
This patch ports Local Accessor and Global Offset passes to the new pass manager. As most of other passes in both AMDGPU and NVPTX backends are still running with the legacy PM, it provides a legacy struct that wraps around the new PM and lets the old interface be used.
Fixes: #5310