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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

Looks good in principle. Could you please simplify the tests (same request as on another commit essentially).



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


================
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
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79021