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

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

whchung marked 2 inline comments 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 {
----------------
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.


================
Comment at: mlir/test/Conversion/GPUToROCDL/memory-attrbution.mlir:1
+// RUN: mlir-opt -allow-unregistered-dialect --convert-gpu-to-rocdl --split-input-file %s | FileCheck %s
+
----------------
ftynse wrote:
> 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.
Sure thing. Let me revise the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79021