[PATCH] D79021: [mlir][rocdl] refactor ROCDL dialect.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Conversion/GPUCommon/GPUOpsLowering.h:27
+  LogicalResult
+  matchAndRewrite(Operation *op, ArrayRef<Value> operands,
+                  ConversionPatternRewriter &rewriter) const override {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79021