Skip to content

[mlir][emitc] Remove copy from scf.for lowering #94898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 10, 2024
Merged

[mlir][emitc] Remove copy from scf.for lowering #94898

merged 1 commit into from
Jun 10, 2024

Conversation

aniragil
Copy link
Contributor

@aniragil aniragil commented Jun 9, 2024

Remove the copy into fresh variables done when lowering scf.for into
emitc.for and use the variables carrying the init and iter values as
the loop's results.

@aniragil
[mlir][emitc] Remove copy from scf.for lowering
ca3d70d 
Remove the copy into fresh variables done when lowering scf.for into
emitc.for and use the variables carrying the init and iter values as
the loop's results.
@llvmbot
Copy link
Member

llvmbot commented Jun 9, 2024 •
edited
Loading

@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: Gil Rapaport (aniragil)

Changes

Remove the copy into fresh variables done when lowering scf.for into
emitc.for and use the variables carrying the init and iter values as
the loop's results.

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

3 Files Affected:

  • (modified) mlir/lib/Conversion/SCFToEmitC/SCFToEmitC.cpp (+3-8)
  • (modified) mlir/test/Conversion/SCFToEmitC/for.mlir (+10-18)
  • (modified) mlir/test/Target/Cpp/for.mlir (-18)
diff --git a/mlir/lib/Conversion/SCFToEmitC/SCFToEmitC.cpp b/mlir/lib/Conversion/SCFToEmitC/SCFToEmitC.cpp
index 367142a520742..0a89242225255 100644
--- a/mlir/lib/Conversion/SCFToEmitC/SCFToEmitC.cpp
+++ b/mlir/lib/Conversion/SCFToEmitC/SCFToEmitC.cpp
@@ -102,10 +102,8 @@ LogicalResult ForLowering::matchAndRewrite(ForOp forOp,
   // assigned to by emitc::assign ops within the loop body.
   SmallVector<Value> resultVariables =
       createVariablesForResults(forOp, rewriter);
-  SmallVector<Value> iterArgsVariables =
-      createVariablesForResults(forOp, rewriter);
 
-  assignValues(forOp.getInits(), iterArgsVariables, rewriter, loc);
+  assignValues(forOp.getInits(), resultVariables, rewriter, loc);
 
   emitc::ForOp loweredFor = rewriter.create<emitc::ForOp>(
       loc, forOp.getLowerBound(), forOp.getUpperBound(), forOp.getStep());
@@ -117,15 +115,12 @@ LogicalResult ForLowering::matchAndRewrite(ForOp forOp,
 
   SmallVector<Value> replacingValues;
   replacingValues.push_back(loweredFor.getInductionVar());
-  replacingValues.append(iterArgsVariables.begin(), iterArgsVariables.end());
+  replacingValues.append(resultVariables.begin(), resultVariables.end());
 
   rewriter.mergeBlocks(forOp.getBody(), loweredBody, replacingValues);
-  lowerYield(iterArgsVariables, rewriter,
+  lowerYield(resultVariables, rewriter,
              cast<scf::YieldOp>(loweredBody->getTerminator()));
 
-  // Copy iterArgs into results after the for loop.
-  assignValues(iterArgsVariables, resultVariables, rewriter, loc);
-
   rewriter.replaceOp(forOp, resultVariables);
   return success();
 }
diff --git a/mlir/test/Conversion/SCFToEmitC/for.mlir b/mlir/test/Conversion/SCFToEmitC/for.mlir
index 7f90310af2189..7e59eac3d4095 100644
--- a/mlir/test/Conversion/SCFToEmitC/for.mlir
+++ b/mlir/test/Conversion/SCFToEmitC/for.mlir
@@ -49,17 +49,13 @@ func.func @for_yield(%arg0 : index, %arg1 : index, %arg2 : index) -> (f32, f32)
 // CHECK-NEXT:    %[[VAL_4:.*]] = arith.constant 1.000000e+00 : f32
 // CHECK-NEXT:    %[[VAL_5:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
 // CHECK-NEXT:    %[[VAL_6:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:    %[[VAL_7:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:    %[[VAL_8:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:    emitc.assign %[[VAL_3]] : f32 to %[[VAL_7]] : f32
-// CHECK-NEXT:    emitc.assign %[[VAL_4]] : f32 to %[[VAL_8]] : f32
+// CHECK-NEXT:    emitc.assign %[[VAL_3]] : f32 to %[[VAL_5]] : f32
+// CHECK-NEXT:    emitc.assign %[[VAL_4]] : f32 to %[[VAL_6]] : f32
 // CHECK-NEXT:    emitc.for %[[VAL_9:.*]] = %[[VAL_0]] to %[[VAL_1]] step %[[VAL_2]] {
-// CHECK-NEXT:      %[[VAL_10:.*]] = arith.addf %[[VAL_7]], %[[VAL_8]] : f32
-// CHECK-NEXT:      emitc.assign %[[VAL_10]] : f32 to %[[VAL_7]] : f32
-// CHECK-NEXT:      emitc.assign %[[VAL_10]] : f32 to %[[VAL_8]] : f32
+// CHECK-NEXT:      %[[VAL_10:.*]] = arith.addf %[[VAL_5]], %[[VAL_6]] : f32
+// CHECK-NEXT:      emitc.assign %[[VAL_10]] : f32 to %[[VAL_5]] : f32
+// CHECK-NEXT:      emitc.assign %[[VAL_10]] : f32 to %[[VAL_6]] : f32
 // CHECK-NEXT:    }
-// CHECK-NEXT:    emitc.assign %[[VAL_7]] : f32 to %[[VAL_5]] : f32
-// CHECK-NEXT:    emitc.assign %[[VAL_8]] : f32 to %[[VAL_6]] : f32
 // CHECK-NEXT:    return %[[VAL_5]], %[[VAL_6]] : f32, f32
 // CHECK-NEXT:  }
 
@@ -78,19 +74,15 @@ func.func @nested_for_yield(%arg0 : index, %arg1 : index, %arg2 : index) -> f32
 // CHECK-SAME:      %[[VAL_0:.*]]: index, %[[VAL_1:.*]]: index, %[[VAL_2:.*]]: index) -> f32 {
 // CHECK-NEXT:    %[[VAL_3:.*]] = arith.constant 1.000000e+00 : f32
 // CHECK-NEXT:    %[[VAL_4:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:    %[[VAL_5:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:    emitc.assign %[[VAL_3]] : f32 to %[[VAL_5]] : f32
+// CHECK-NEXT:    emitc.assign %[[VAL_3]] : f32 to %[[VAL_4]] : f32
 // CHECK-NEXT:    emitc.for %[[VAL_6:.*]] = %[[VAL_0]] to %[[VAL_1]] step %[[VAL_2]] {
 // CHECK-NEXT:      %[[VAL_7:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:      %[[VAL_8:.*]] = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
-// CHECK-NEXT:      emitc.assign %[[VAL_5]] : f32 to %[[VAL_8]] : f32
+// CHECK-NEXT:      emitc.assign %[[VAL_4]] : f32 to %[[VAL_7]] : f32
 // CHECK-NEXT:      emitc.for %[[VAL_9:.*]] = %[[VAL_0]] to %[[VAL_1]] step %[[VAL_2]] {
-// CHECK-NEXT:        %[[VAL_10:.*]] = arith.addf %[[VAL_8]], %[[VAL_8]] : f32
-// CHECK-NEXT:        emitc.assign %[[VAL_10]] : f32 to %[[VAL_8]] : f32
+// CHECK-NEXT:        %[[VAL_10:.*]] = arith.addf %[[VAL_7]], %[[VAL_7]] : f32
+// CHECK-NEXT:        emitc.assign %[[VAL_10]] : f32 to %[[VAL_7]] : f32
 // CHECK-NEXT:      }
-// CHECK-NEXT:      emitc.assign %[[VAL_8]] : f32 to %[[VAL_7]] : f32
-// CHECK-NEXT:      emitc.assign %[[VAL_7]] : f32 to %[[VAL_5]] : f32
+// CHECK-NEXT:      emitc.assign %[[VAL_7]] : f32 to %[[VAL_4]] : f32
 // CHECK-NEXT:    }
-// CHECK-NEXT:    emitc.assign %[[VAL_5]] : f32 to %[[VAL_4]] : f32
 // CHECK-NEXT:    return %[[VAL_4]] : f32
 // CHECK-NEXT:  }
diff --git a/mlir/test/Target/Cpp/for.mlir b/mlir/test/Target/Cpp/for.mlir
index 60988bcb46556..af1d829113f9d 100644
--- a/mlir/test/Target/Cpp/for.mlir
+++ b/mlir/test/Target/Cpp/for.mlir
@@ -40,8 +40,6 @@ func.func @test_for_yield() {
   %s0 = "emitc.constant"() <{value = 0 : i32}> : () -> i32
   %p0 = "emitc.constant"() <{value = 1.0 : f32}> : () -> f32
 
-  %0 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
-  %1 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
   %2 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
   %3 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
   emitc.assign %s0 : i32 to %2 : i32
@@ -53,8 +51,6 @@ func.func @test_for_yield() {
     emitc.assign %pn : f32 to %3 : f32
     emitc.yield
   }
-  emitc.assign %2 : i32 to %0 : i32
-  emitc.assign %3 : f32 to %1 : f32
 
   return
 }
@@ -64,8 +60,6 @@ func.func @test_for_yield() {
 // CPP-DEFAULT-NEXT: size_t [[STEP:[^ ]*]] = 1;
 // CPP-DEFAULT-NEXT: int32_t [[S0:[^ ]*]] = 0;
 // CPP-DEFAULT-NEXT: float [[P0:[^ ]*]] = 1.000000000e+00f;
-// CPP-DEFAULT-NEXT: int32_t [[SE:[^ ]*]];
-// CPP-DEFAULT-NEXT: float [[PE:[^ ]*]];
 // CPP-DEFAULT-NEXT: int32_t [[SI:[^ ]*]];
 // CPP-DEFAULT-NEXT: float [[PI:[^ ]*]];
 // CPP-DEFAULT-NEXT: [[SI:[^ ]*]] = [[S0]];
@@ -76,8 +70,6 @@ func.func @test_for_yield() {
 // CPP-DEFAULT-NEXT: [[SI]] = [[SN]];
 // CPP-DEFAULT-NEXT: [[PI]] = [[PN]];
 // CPP-DEFAULT-NEXT: }
-// CPP-DEFAULT-NEXT: [[SE]] = [[SI]];
-// CPP-DEFAULT-NEXT: [[PE]] = [[PI]];
 // CPP-DEFAULT-NEXT: return;
 
 // CPP-DECLTOP: void test_for_yield() {
@@ -86,8 +78,6 @@ func.func @test_for_yield() {
 // CPP-DECLTOP-NEXT: size_t [[STEP:[^ ]*]];
 // CPP-DECLTOP-NEXT: int32_t [[S0:[^ ]*]];
 // CPP-DECLTOP-NEXT: float [[P0:[^ ]*]];
-// CPP-DECLTOP-NEXT: int32_t [[SE:[^ ]*]];
-// CPP-DECLTOP-NEXT: float [[PE:[^ ]*]];
 // CPP-DECLTOP-NEXT: int32_t [[SI:[^ ]*]];
 // CPP-DECLTOP-NEXT: float [[PI:[^ ]*]];
 // CPP-DECLTOP-NEXT: int32_t [[SN:[^ ]*]];
@@ -99,8 +89,6 @@ func.func @test_for_yield() {
 // CPP-DECLTOP-NEXT: [[P0]] = 1.000000000e+00f;
 // CPP-DECLTOP-NEXT: ;
 // CPP-DECLTOP-NEXT: ;
-// CPP-DECLTOP-NEXT: ;
-// CPP-DECLTOP-NEXT: ;
 // CPP-DECLTOP-NEXT: [[SI:[^ ]*]] = [[S0]];
 // CPP-DECLTOP-NEXT: [[PI:[^ ]*]] = [[P0]];
 // CPP-DECLTOP-NEXT: for (size_t [[ITER:[^ ]*]] = [[START]]; [[ITER]] < [[STOP]]; [[ITER]] += [[STEP]]) {
@@ -109,8 +97,6 @@ func.func @test_for_yield() {
 // CPP-DECLTOP-NEXT: [[SI]] = [[SN]];
 // CPP-DECLTOP-NEXT: [[PI]] = [[PN]];
 // CPP-DECLTOP-NEXT: }
-// CPP-DECLTOP-NEXT: [[SE]] = [[SI]];
-// CPP-DECLTOP-NEXT: [[PE]] = [[PI]];
 // CPP-DECLTOP-NEXT: return;
 
 func.func @test_for_yield_2() {
@@ -121,8 +107,6 @@ func.func @test_for_yield_2() {
   %s0 = emitc.literal "0" : i32
   %p0 = emitc.literal "M_PI" : f32
 
-  %0 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
-  %1 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
   %2 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
   %3 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> f32
   emitc.assign %s0 : i32 to %2 : i32
@@ -134,8 +118,6 @@ func.func @test_for_yield_2() {
     emitc.assign %pn : f32 to %3 : f32
     emitc.yield
   }
-  emitc.assign %2 : i32 to %0 : i32
-  emitc.assign %3 : f32 to %1 : f32
 
   return
 }

@aniragil aniragil mentioned this pull request Jun 9, 2024
Open
cferry-AMD
cferry-AMD approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@cferry-AMD cferry-AMD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sound to me.

simon-camp
simon-camp approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@simon-camp simon-camp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@aniragil aniragil merged commit 8b7e836 into llvm:main Jun 10, 2024
10 checks passed
@aniragil aniragil deleted the emitc-remove-for-result-copy branch June 10, 2024 10:58
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marbre marbre Awaiting requested review from marbre

@simon-camp simon-camp Awaiting requested review from simon-camp

@mgehre-amd mgehre-amd Awaiting requested review from mgehre-amd

@cferry-AMD cferry-AMD Awaiting requested review from cferry-AMD

@TinaAMD TinaAMD Awaiting requested review from TinaAMD

Assignees
No one assigned
Labels
mlir:emitc mlir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aniragil @llvmbot @simon-camp @cferry-AMD