- Extract common logic between -convert-gpu-to-nvvm and -convert-gpu-to-rocdl.
- Cope with the fact that alloca operates on different addrspaces between NVVM and ROCDL.
- Modernize unit tests for ROCDL dialect.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h | ||
---|---|---|
28 | Can you break this function up? It is fairly large. |
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h | ||
---|---|---|
28 | This file is the identical copy from mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp , with only 2 places changed: struct GPUFuncOpLowering : ConvertToLLVMPattern { explicit GPUFuncOpLowering(LLVMTypeConverter &typeConverter) becomes template <unsigned AllocaAddrSpace> struct GPUFuncOpLowering : ConvertToLLVMPattern { explicit GPUFuncOpLowering(LLVMTypeConverter &typeConverter) auto ptrType = typeConverter.convertType(type.getElementType()) .cast<LLVM::LLVMType>() .getPointerTo(); becomes auto ptrType = typeConverter.convertType(type.getElementType()) .cast<LLVM::LLVMType>() .getPointerTo(AllocaAddrSpace); |
@rriddle Could you help elaborate how you envision GPUFuncOpLowering::matchAndRewrite be broken up? I'm less inclined to so as:
- The logic is directly moved from LowerGpuOpsToNVVMOps.cpp as it's mostly common between NVVM and ROCDL dialect, other than the fact that alloca takes place on different address spaces. And additional logic and tests were already introduced in the patch.
- The logic is self-contained with no additional consumers.
Looks good in principle. Could you please simplify the tests (same request as on another commit essentially).
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h | ||
---|---|---|
28 | Thanks for highlighting the fact of code motion @whchung ! Since this commit only moves the code, I would suggest to keep the code as is and split it in a separate commit. | |
mlir/test/Conversion/GPUToROCDL/memory-attrbution.mlir | ||
1 ↗ | (On Diff #260728) | Is this anyhow different from GPUtoNVVM/memory-attribution? I suppose the alloca part has different address space now. I would suggest putting tests that are common for both conversions into test/Conversion/GPUCommon (similarly to code), and only keeping here the parts that differ. |
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h | ||
---|---|---|
28 | @ftynse It may not be doable to separate the change into another commit as:
Should we keep this code as-is then ROCDL tests would break and defeats the purpose of this patch. | |
mlir/test/Conversion/GPUToROCDL/memory-attrbution.mlir | ||
1 ↗ | (On Diff #260728) | Sure thing. Let me revise the patch. |
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h | ||
---|---|---|
28 | Ah, keeping this revision NFC sounds good to me. Splitting in a followup is good. |
Refactor the patch per review comments to make it NFC.
Also merge memory-attributes.mlir between NVVM and ROCDL into one test.
I think this broke the shared library build: https://buildkite.com/mlir/mlir-core/builds/4565#913f7085-2369-4b8a-a4c8-fe62ff728837
Can you break this function up? It is fairly large.