This is an archive of the discontinued LLVM Phabricator instance.

[mlir][nvvm][rocdl] refactor NVVM and ROCDL dialect. NFC.
ClosedPublic

Authored by whchung on Apr 28 2020, 9:54 AM.

Details

Summary
  • 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.

Diff Detail

Event Timeline

whchung created this revision.Apr 28 2020, 9:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
whchung added a project: Restricted Project.Apr 28 2020, 9:56 AM
rriddle added inline comments.Apr 28 2020, 10:34 AM
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h
28

Can you break this function up? It is fairly large.

whchung marked an inline comment as done.Apr 28 2020, 10:51 AM
whchung added inline comments.
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);
whchung updated this revision to Diff 260728.Apr 28 2020, 12:24 PM

Address issues in the build checks.

@rriddle Could you help elaborate how you envision GPUFuncOpLowering::matchAndRewrite be broken up? I'm less inclined to so as:

  1. 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.
  2. The logic is self-contained with no additional consumers.
ftynse accepted this revision.Apr 29 2020, 9:12 AM

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.

This revision is now accepted and ready to land.Apr 29 2020, 9:12 AM
whchung marked 2 inline comments as done.Apr 29 2020, 9:57 AM
whchung added inline comments.
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h
28

@ftynse It may not be doable to separate the change into another commit as:

  • In convert-gpu-to-nvvm it now uses GPUFuncOpLowering<0>
  • In convert-gpu-to-rocdl it now uses GPUFuncOpLowering<5>

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.

rriddle added inline comments.Apr 29 2020, 10:17 AM
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h
28

Ah, keeping this revision NFC sounds good to me. Splitting in a followup is good.

whchung updated this revision to Diff 261007.Apr 29 2020, 1:22 PM

Refactor the patch per review comments to make it NFC.

Also merge memory-attributes.mlir between NVVM and ROCDL into one test.

whchung retitled this revision from [mlir][rocdl] refactor ROCDL dialect. to [mlir][nvvm][rocdl] refactor NVVM and ROCDL dialect. NFC..Apr 29 2020, 1:23 PM
whchung edited the summary of this revision. (Show Details)
whchung updated this revision to Diff 261008.Apr 29 2020, 1:26 PM
whchung marked 2 inline comments as done.

Fix comments.

whchung marked an inline comment as done.Apr 29 2020, 1:28 PM
whchung added inline comments.
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h
28

@ftynse @rriddle I've revised the patch so it seems to be NFC from my perspective. Could you give this patch another round of review? Thanks.

I'll submit another patch which adds rocdl.barrier and hook it up with gpu.barrier.

whchung updated this revision to Diff 261035.Apr 29 2020, 2:16 PM

Address build errors.

This revision was automatically updated to reflect the committed changes.

@jpienaar Should I submit the fix in a new patch, or can I update this one?

@jpienaar I found I couldn't revise this patch as it was closed already. I've submitted D79225 to fix shared lib build. Could you help get it merged once tests are passing? Thanks for sorry for bringing your confusion.

mlir/test/Conversion/GPUCommon/memory-attrbution.mlir