[PATCH] D79021: [mlir][nvvm][rocdl] refactor NVVM and ROCDL dialect. NFC.

[Wen-Heng (Jack) Chung via Phabricator via llvm-commits]

whchung marked an inline comment as done.
whchung added inline comments.


================
Comment at: mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h:27
+  LogicalResult
+  matchAndRewrite(Operation *op, ArrayRef<Value> operands,
+                  ConversionPatternRewriter &rewriter) const override {
----------------
rriddle wrote:
> whchung wrote:
> > ftynse wrote:
> > > whchung wrote:
> > > > rriddle wrote:
> > > > > Can you break this function up? It is fairly large.
> > > > This file is the identical copy from `mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp` , with only 2 places changed:
> > > > 
> > > > 1) 
> > > > ```
> > > > struct GPUFuncOpLowering : ConvertToLLVMPattern {
> > > >   explicit GPUFuncOpLowering(LLVMTypeConverter &typeConverter)
> > > > ```
> > > > 
> > > > becomes
> > > > 
> > > > ```
> > > > template <unsigned AllocaAddrSpace>
> > > > struct GPUFuncOpLowering : ConvertToLLVMPattern {
> > > >   explicit GPUFuncOpLowering(LLVMTypeConverter &typeConverter)
> > > > ```
> > > > 
> > > > 2)
> > > > 
> > > > ```
> > > >         auto ptrType = typeConverter.convertType(type.getElementType())
> > > >                            .cast<LLVM::LLVMType>()
> > > >                            .getPointerTo();
> > > > ```
> > > > 
> > > > becomes
> > > > 
> > > > ```
> > > >         auto ptrType = typeConverter.convertType(type.getElementType())
> > > >                            .cast<LLVM::LLVMType>()
> > > >                            .getPointerTo(AllocaAddrSpace);
> > > > ```
> > > 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.
> > @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.
> Ah, keeping this revision NFC sounds good to me. Splitting in a followup is good.
@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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79021/new/

https://reviews.llvm.org/D79021