[MLIR] Added check for IsTerminator trait #79317

d-smirnov · 2024-01-24T16:09:01Z

This PR adds a check for IsTerminator trait to prevent deletion of ops like gpu.terminator as a "simple op" by RemoveDeadValues pass.

d-smirnov · 2024-01-24T16:09:20Z

@GeorgeARM

llvmbot · 2024-01-24T16:09:31Z

@llvm/pr-subscribers-mlir-core

Author: Dmitriy Smirnov (d-smirnov)

Changes

This PR adds a check for IsTerminator trait to prevent deletion of ops like gpu.terminator as a "simple op" by RemoveDeadValues pass.

Full diff: https://github.com/llvm/llvm-project/pull/79317.diff

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+4)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+22)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index d80e514aed36f8..2774903f032848 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -598,6 +598,10 @@ void RemoveDeadValues::runOnOperation() {
       // Nothing to do because this terminator is associated with either a
       // function op or a region branch op and gets cleaned when these ops are
       // cleaned.
+    } else if (op->hasTrait<::mlir::OpTrait::IsTerminator>()) {
+      // Nothing to do because this terminator is associated with either a
+      // function op or a region branch op and gets cleaned when these ops are
+      // cleaned.
     } else if (isa<RegionBranchTerminatorOpInterface>(op)) {
       // Nothing to do because this terminator is associated with a region
       // branch op and gets cleaned when the latter is cleaned.
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 600810a785b1f9..69426fdb620832 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -335,3 +335,25 @@ func.func @main(%arg3 : i32, %arg4 : i1) {
   %non_live_0 = func.call @clean_region_branch_op_erase_it(%arg3, %arg4) : (i32, i1) -> (i32)
   return
 }
+
+// -----
+
+#map = affine_map<(d0)[s0, s1] -> (d0 * s0 + s1)>
+func.func @kernel(%arg0: memref<18xf32>) {
+  %c1 = arith.constant 1 : index
+  %c18 = arith.constant 18 : index
+  gpu.launch blocks(%arg3, %arg4, %arg5) in (%arg9 = %c18, %arg10 = %c18, %arg11 = %c18) threads(%arg6, %arg7, %arg8) in (%arg12 = %c1, %arg13 = %c1, %arg14 = %c1) {
+    %c1_0 = arith.constant 1 : index
+    %c0_1 = arith.constant 0 : index
+    %cst_2 = arith.constant 25.4669495 : f32
+    %6 = affine.apply #map(%arg3)[%c1_0, %c0_1]
+    memref.store %cst_2, %arg0[%6] : memref<18xf32>
+    gpu.terminator
+  } {SCFToGPU_visited}
+  return
+}
+
+// CHECK-LABEL: func.func @kernel(%arg0: memref<18xf32>) {
+// CHECK: gpu.launch blocks
+// CHECK: memref.store
+// CHECK-NEXT: gpu.terminator

llvmbot · 2024-01-24T16:09:32Z

@llvm/pr-subscribers-mlir

Author: Dmitriy Smirnov (d-smirnov)

Changes

This PR adds a check for IsTerminator trait to prevent deletion of ops like gpu.terminator as a "simple op" by RemoveDeadValues pass.

Full diff: https://github.com/llvm/llvm-project/pull/79317.diff

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+4)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+22)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index d80e514aed36f88..2774903f032848a 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -598,6 +598,10 @@ void RemoveDeadValues::runOnOperation() {
       // Nothing to do because this terminator is associated with either a
       // function op or a region branch op and gets cleaned when these ops are
       // cleaned.
+    } else if (op->hasTrait<::mlir::OpTrait::IsTerminator>()) {
+      // Nothing to do because this terminator is associated with either a
+      // function op or a region branch op and gets cleaned when these ops are
+      // cleaned.
     } else if (isa<RegionBranchTerminatorOpInterface>(op)) {
       // Nothing to do because this terminator is associated with a region
       // branch op and gets cleaned when the latter is cleaned.
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 600810a785b1f9e..69426fdb6208329 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -335,3 +335,25 @@ func.func @main(%arg3 : i32, %arg4 : i1) {
   %non_live_0 = func.call @clean_region_branch_op_erase_it(%arg3, %arg4) : (i32, i1) -> (i32)
   return
 }
+
+// -----
+
+#map = affine_map<(d0)[s0, s1] -> (d0 * s0 + s1)>
+func.func @kernel(%arg0: memref<18xf32>) {
+  %c1 = arith.constant 1 : index
+  %c18 = arith.constant 18 : index
+  gpu.launch blocks(%arg3, %arg4, %arg5) in (%arg9 = %c18, %arg10 = %c18, %arg11 = %c18) threads(%arg6, %arg7, %arg8) in (%arg12 = %c1, %arg13 = %c1, %arg14 = %c1) {
+    %c1_0 = arith.constant 1 : index
+    %c0_1 = arith.constant 0 : index
+    %cst_2 = arith.constant 25.4669495 : f32
+    %6 = affine.apply #map(%arg3)[%c1_0, %c0_1]
+    memref.store %cst_2, %arg0[%6] : memref<18xf32>
+    gpu.terminator
+  } {SCFToGPU_visited}
+  return
+}
+
+// CHECK-LABEL: func.func @kernel(%arg0: memref<18xf32>) {
+// CHECK: gpu.launch blocks
+// CHECK: memref.store
+// CHECK-NEXT: gpu.terminator

matthias-springer · 2024-01-24T16:38:04Z

mlir/lib/Transforms/RemoveDeadValues.cpp

@@ -598,6 +598,10 @@ void RemoveDeadValues::runOnOperation() {
      // Nothing to do because this terminator is associated with either a
      // function op or a region branch op and gets cleaned when these ops are
      // cleaned.
+    } else if (op->hasTrait<::mlir::OpTrait::IsTerminator>()) {
+      // Nothing to do because this terminator is associated with either a


This comment does not really fit because other ops (that are not function ops and do not implement the region branch op interface) can also have terminators.

What happened to the test case before this change? Did the pass produce invalid IR that does not verify because the terminator is missing?

Before the patch gpu.terminator was just removed by cleanSimpleOp method producing invalid IR

Updated comment

joker-eph · 2024-01-24T21:00:06Z

mlir/lib/Transforms/RemoveDeadValues.cpp

@@ -598,6 +598,9 @@ void RemoveDeadValues::runOnOperation() {
      // Nothing to do because this terminator is associated with either a
      // function op or a region branch op and gets cleaned when these ops are
      // cleaned.
+    } else if (op->hasTrait<::mlir::OpTrait::IsTerminator>()) {
+      // Nothing to do here because this a terminator op and it should be
+      // honored with respect to its parent


Seems in line with the cleanSimpleOp comment that states:

/// It is assumed that `op` is simple. Here, a simple op is one which isn't a /// symbol op, a symbol-user op, a region branch op, a branch op, a region /// branch terminator op, or return-like.

Now isn't this check making obsolete the two others checks for ReturnLike and RegionBranchTerminatorOpInterface? There two should also be Terminator.

The check seems completely relevant for gpu.termnator op as it declared as:

952 def GPU_TerminatorOp : GPU_Op<"terminator", [HasParent<"LaunchOp">,
953 Pure, Terminator]>,
954 Arguments<(ins)>, Results<(outs)> {

Same for gpu.return op

Which check are you referring to? The new one?

You may have read it the opposite of what I wrote: I am referring to the existing checks being obsolete after this patch.

Sorry, misread you.

OK, so: can we remove them?

Add check for IsTerminator trait to prevent deletion of ops like gpu.terminator as a "simple op" by RemoveDeadValues pass.

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jan 24, 2024

matthias-springer reviewed Jan 24, 2024

View reviewed changes

d-smirnov force-pushed the terminator branch from 734299a to 5aa0396 Compare January 24, 2024 17:42

joker-eph reviewed Jan 24, 2024

View reviewed changes

d-smirnov force-pushed the terminator branch from 5aa0396 to c982d99 Compare January 25, 2024 10:11

d-smirnov requested review from matthias-springer and joker-eph January 25, 2024 11:29

d-smirnov added 2 commits January 25, 2024 21:14

[MLIR] Added check for IsTerminator trait

41ff24c

Add check for IsTerminator trait to prevent deletion of ops like gpu.terminator as a "simple op" by RemoveDeadValues pass.

Removed obsolete checks

bd371f8

d-smirnov force-pushed the terminator branch from 8eb182e to bd371f8 Compare January 25, 2024 21:14

joker-eph approved these changes Jan 25, 2024

View reviewed changes

GeorgeARM merged commit 65fd055 into llvm:main Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MLIR] Added check for IsTerminator trait #79317

[MLIR] Added check for IsTerminator trait #79317

Uh oh!

d-smirnov commented Jan 24, 2024

Uh oh!

d-smirnov commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024

Uh oh!

matthias-springer Jan 24, 2024

Uh oh!

d-smirnov Jan 24, 2024

Uh oh!

d-smirnov Jan 24, 2024

Uh oh!

joker-eph Jan 24, 2024 •

edited

Loading

Uh oh!

d-smirnov Jan 24, 2024

Uh oh!

joker-eph Jan 24, 2024

Uh oh!

d-smirnov Jan 24, 2024

Uh oh!

joker-eph Jan 25, 2024

Uh oh!

d-smirnov Jan 25, 2024

Uh oh!

Uh oh!

[MLIR] Added check for IsTerminator trait #79317

[MLIR] Added check for IsTerminator trait #79317

Uh oh!

Conversation

d-smirnov commented Jan 24, 2024

Uh oh!

d-smirnov commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024

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 Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

joker-eph Jan 24, 2024 •

edited

Loading