[ROCm] Add ROCDL Dialect. #63

whchung · 2019-08-02T16:20:38Z

Add supporting logic to lower MLIR module into ROCm-Device-Libs (ROCDL) IR.

Follow-up to #57.

River707 · 2019-08-02T17:13:25Z

include/mlir/LLVMIR/ROCDLDialect.h

+
+#include "mlir/IR/Dialect.h"
+#include "mlir/IR/OpDefinition.h"
+namespace mlir {


Add a newline after the includes.

Will revise.

River707 · 2019-08-02T17:14:30Z

include/mlir/LLVMIR/ROCDLDialect.h

+
+class ROCDLDialect : public Dialect {
+public:
+  explicit ROCDLDialect(MLIRContext *context);


You'll likely want to provide a static 'getDialectNamespace' utility. This is used by several template mechanisms:
static StringRef getDialectNamespace() { return "..."; }

Will revise.

River707 · 2019-08-02T17:18:46Z

lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

+
+#include "llvm/ADT/StringSwitch.h"
+
+namespace mlir {


Drop the outer 'mlir' namespace.

Will revise.

River707 · 2019-08-02T17:19:15Z

lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

+
+} // anonymous namespace
+
+FunctionPassBase *createLowerGpuOpsToROCDLOpsPass() {


This function should be qualified with mlir:: and not within a namespace.

Will revise.

River707 · 2019-08-02T17:20:01Z

lib/LLVMIR/IR/ROCDLDialect.cpp

+#include "llvm/Support/SourceMgr.h"
+
+namespace mlir {
+namespace ROCDL {


Don't place these within namespaces, prefer explicit qualification + using directives.

Will revise.

River707 · 2019-08-02T17:20:55Z

lib/Target/LLVMIR/ConvertToROCDLIR.cpp

+using namespace mlir;
+
+namespace {
+static llvm::Value *createIntrinsicCall(llvm::IRBuilder<> &builder,


static methods should be in the top-level namespace.

Will revise.

River707 · 2019-08-02T17:22:10Z

test/LLVMIR/rocdl.mlir

+// RUN: mlir-opt %s | FileCheck %s
+
+func @rocdl_special_regs() -> !llvm.i32 {
+  // CHECK: %0 = rocdl.workitem.id.x : !llvm.i32


Don't check SSA value names directly in tests:
https://github.com/tensorflow/mlir/blob/master/g3doc/TestingGuide.md

@River707 I was following examples depicted at:
https://github.com/tensorflow/mlir/blob/master/test/LLVMIR/nvvm.mlir

How do you recommend to revise the unit test?

For here, it should be enough to just drop the SSA value names, e.g. "%0", from the CHECK lines. The CHECK should as constrained as possible and only focus on testing what is important. The SSA value names here are not related to what you are trying to check, i.e. the custom syntax of the ops.

Thanks. Will revise.

River707 · 2019-08-02T17:22:19Z

test/Target/rocdlir.mlir

+// RUN: mlir-translate -mlir-to-rocdlir %s | FileCheck %s
+
+func @rocdl_special_regs() -> !llvm.i32 {
+  // CHECK: %1 = call i32 @llvm.amdgcn.workitem.id.x()


Will revise.

River707 · 2019-08-02T17:23:01Z

Make sure to squash all of your changes into one commit.

whchung · 2019-08-02T17:38:25Z

@River707 Thanks for your quick review! Pretty much all the issues you pointed out are in NVVM dialect as well. Should I file another PR to change it?

River707 · 2019-08-02T17:59:53Z

Is your question if you should file a PR to cleanup the NVVM dialect? If so that would be awesome, it needs to be cleaned up as well.

joker-eph · 2019-08-02T17:42:49Z

lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

+// =============================================================================
+//
+// This file implements a pass to generate ROCDLIR operations for higher-level
+// GPU operations.


This seems like independent from the definition of the dialect, you should be able to split this in a followup PR

ok will split this PR into smaller pieces and file this module as a separate PR.

joker-eph · 2019-08-02T18:00:51Z

lib/Target/LLVMIR/ConvertToROCDLIR.cpp

+// =============================================================================
+//
+// This file implements a translation between the MLIR LLVM + ROCDL dialects and
+// LLVM IR with ROCDL intrinsics and metadata.


This seems like another component that I rather review in a separate followup PR from the introduction of the dialect itself.

ok will split this PR into smaller pieces and file this module as a separate PR.

joker-eph · 2019-08-02T18:48:33Z

test/Target/rocdlir.mlir

+}
+
+// This function has the "amdgpu_kernel" calling convention after conversion.
+// CHECK:     amdgpu_kernel


Nit (not super important here, but you may write more tests in the future): in general we separate multiple functions testing with CHECK-LABEL: directives, this helps understand failures when the test fail (and makes FileCheck a bit more robust).

antiagainst · 2019-08-02T19:15:53Z

include/mlir/LLVMIR/ROCDLDialect.h

+// limitations under the License.
+// =============================================================================
+//
+// This file defines the ROCDL IR dialect in MLIR, containing ROCDL operations and


Nit: this line needs to be wrapped.

antiagainst · 2019-08-02T19:21:17Z

include/mlir/LLVMIR/ROCDLOps.td

+include "mlir/LLVMIR/LLVMOpBase.td"
+
+def ROCDL_Dialect : Dialect {
+  let name = "rocdl";


Adding a description regarding this dialect would be nice.

antiagainst · 2019-08-02T19:21:33Z

include/mlir/LLVMIR/ROCDLOps.td

+}
+
+class ROCDL_Op<string mnemonic, list<OpTrait> traits = []> :
+  LLVM_OpBase<ROCDL_Dialect, mnemonic, traits> {


No need to have the empty {}.

antiagainst · 2019-08-02T19:22:51Z

include/mlir/LLVMIR/ROCDLOps.td

+def ROCDL_BlockIdYOp : ROCDL_SpecialRegisterOp<"workgroup.id.y">;
+def ROCDL_BlockIdZOp : ROCDL_SpecialRegisterOp<"workgroup.id.z">;
+
+class ROCDL_DeviceFunctionOp<string mnemonic, string device_function, int parameter,


Nit: can we format this class a bit nicer (proper line wrapping, etc.)?

antiagainst · 2019-08-02T19:23:56Z

include/mlir/Target/ROCDLIR.h

+// limitations under the License.
+// =============================================================================
+//
+// This file declares the entry point for the MLIR to LLVM + ROCDL IR conversion.


Nit: need wrapping here.

antiagainst · 2019-08-02T19:26:50Z

lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

+  // XOp, YOp and ZOp are assumed to return an `llvm.i32` value.  Depending on
+  // `indexBitwidth`, sign-extend or truncate the resulting value to match the
+  // bitwidth expected by the consumers of the value.
+  template <typename XOp, typename YOp, typename ZOp, class Op>


This file need to be clang-formated.

antiagainst · 2019-08-02T19:27:49Z

lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

+      newOp = builder.create<ZOp>(loc, LLVM::LLVMType::getInt32Ty(dialect));
+      break;
+    default:
+      operation.emitError("Illegal dimension: " + operation.dimension());


The convention is to start error messages with lowercase letters.

antiagainst · 2019-08-02T19:29:42Z

lib/LLVMIR/IR/ROCDLDialect.cpp

+      parser->parseColonType(type))
+    return failure();
+
+  result->addTypes(type);


I think we can do return parser->addTypeToList(type, state->types); here.

joker-eph · 2019-08-28T10:32:02Z

Hey @whchung, any update?

joker-eph · 2019-09-06T05:27:21Z

Closing for now since there hasn't been any activity for a month. Feel free to re-open as needed!

whchung · 2019-09-06T14:10:58Z

@joker-eph sorry I was focusing on internal tools based on mlir-rocm-runner lately. I'll revisit the PRs I submitted next week.

joker-eph · 2019-09-07T16:48:58Z

Good to hear! No hurry, I just closed to keep our open pull-requests count lower as we try to keep an eye on all the active ones.

Initial commit to add ROCDL Dialect.

a0e4b8e

googlebot added the cla: yes label Aug 2, 2019

Merge branch 'master' into exp-mlir-rocm-rocdl

7827734

River707 suggested changes Aug 2, 2019

View reviewed changes

joker-eph suggested changes Aug 2, 2019

View reviewed changes

joker-eph reviewed Aug 2, 2019

View reviewed changes

antiagainst reviewed Aug 2, 2019

View reviewed changes

joker-eph closed this Sep 6, 2019

This was referenced Sep 26, 2019

[ROCm] Adding ROCDL Dialect. #146

Closed

[ROCm] Adding pass to lower GPU Dialect to ROCDL Dialect. #154

Closed

deven-amd mentioned this pull request Oct 10, 2019

[ROCm] Adding pass to generate the HSACO binary blob from the GPU kernel function #181

Open

bondhugula mentioned this pull request Oct 11, 2019

IR printing (-print-ir-after-all) crash #182

Closed


		} // anonymous namespace

		FunctionPassBase *createLowerGpuOpsToROCDLOpsPass() {

[ROCm] Add ROCDL Dialect. #63

[ROCm] Add ROCDL Dialect. #63

Uh oh!

Conversation

whchung commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

River707 commented Aug 2, 2019

Uh oh!

whchung commented Aug 2, 2019

Uh oh!

River707 commented Aug 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph commented Aug 28, 2019

Uh oh!

joker-eph commented Sep 6, 2019

Uh oh!

whchung commented Sep 6, 2019

whchung commented Aug 2, 2019 •

edited

Loading