[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Sergio Afonso via Phabricator via cfe-commits]

skatrak abandoned this revision.
skatrak added a comment.

I'll close this patch now, since the agreed approach has been to try using the 
`IsolatedFromAbove` trait and not to add an interface modifying the CSE pass 
behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159212

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Sergio Afonso via Phabricator via cfe-commits]

skatrak added a comment.

Thank you for your comments @jsjodin and @kiranchandramohan , I created an RFC 
here 

 to try to get consensus on what the best solution should be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159212

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I think this requires an RFC in MLIR discourse to get some feedback from the 
experts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159212

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Jan Sjödin via Phabricator via cfe-commits]

jsjodin added a comment.

I'm not sure if we want to add an interface for every transform. Is there a way 
to have a generic solution to this,  like a new trait or more general 
interface? It would be nice to avoid the case where a new transform will have 
to touch all the dialects, or a new op will require implementing/modifying a 
bunch of interfaces expressing basically the same thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159212

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Sergio Afonso via Phabricator via cfe-commits]

skatrak created this revision.
skatrak added reviewers: ftynse, kiranchandramohan, jsjodin, domada, agozillon, 
raghavendhra, TIFitis, shraiysh.
Herald added subscribers: bviyer, Moerafaat, zero9178, bzcheeseman, sdasgup3, 
wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, 
Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, 
antiagainst, shauheen, rriddle, mehdi_amini.
Herald added a project: All.
skatrak requested review of this revision.
Herald added a reviewer: nicolasvasilache.
Herald added subscribers: cfe-commits, stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, MLIR.

This patch adds the `DialectCSEInterface`, which dialects can implement and 
register to prevent the common sub-expression elimination (CSE) pass from 
modifying regions of certain operations.

The result is that these operations would be treated by CSE as if they were 
`IsolatedFromAbove`, but without the restrictions that come with that trait.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159212

Files:
  clang/docs/tools/clang-formatted-files.txt
  mlir/include/mlir/Interfaces/CSEInterfaces.h
  mlir/lib/Transforms/CSE.cpp
  mlir/test/Transforms/cse.mlir
  mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp
  mlir/test/lib/Dialect/Test/TestOps.td

Index: mlir/test/lib/Dialect/Test/TestOps.td
===
--- mlir/test/lib/Dialect/Test/TestOps.td
+++ mlir/test/lib/Dialect/Test/TestOps.td
@@ -2703,6 +2703,10 @@
   }];
 }
 
+def NoCSEOneRegionOp : TEST_Op<"no_cse_one_region_op", []> {
+  let regions = (region AnyRegion);
+}
+
 //===--===//
 // Test Ops to upgrade base on the dialect versions
 //===--===//
Index: mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp
===
--- mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp
+++ mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "TestDialect.h"
+#include "mlir/Interfaces/CSEInterfaces.h"
 #include "mlir/Interfaces/FoldInterfaces.h"
 #include "mlir/Reducer/ReductionPatternInterface.h"
 #include "mlir/Transforms/InliningUtils.h"
@@ -273,6 +274,16 @@
   }
 };
 
+struct TestDialectCSEInterface : public DialectCSEInterface {
+  using DialectCSEInterface::DialectCSEInterface;
+
+  bool subexpressionExtractionAllowed(Operation *op) const final {
+// Don't allow extracting common subexpressions from the region of these
+// operations.
+return !isa(op);
+  }
+};
+
 /// This class defines the interface for handling inlining with standard
 /// operations.
 struct TestInlinerInterface : public DialectInlinerInterface {
@@ -385,6 +396,7 @@
   auto &blobInterface = addInterface();
   addInterface(blobInterface);
 
-  addInterfaces();
+  addInterfaces();
 }
Index: mlir/test/Transforms/cse.mlir
===
--- mlir/test/Transforms/cse.mlir
+++ mlir/test/Transforms/cse.mlir
@@ -520,3 +520,23 @@
   %2 = "test.op_with_memread"() : () -> (i32)
   return %0, %2, %1 : i32, i32, i32
 }
+
+// CHECK-LABEL: @no_cse_across_disabled_op
+func.func @no_cse_across_disabled_op() -> (i32) {
+  // CHECK-NEXT: %[[CONST1:.+]] = arith.constant 1 : i32
+  %0 = arith.constant 1 : i32
+
+  // CHECK-NEXT: test.no_cse_one_region_op
+  "test.no_cse_one_region_op"() ({
+%1 = arith.constant 1 : i32
+%2 = arith.addi %1, %1 : i32
+"foo.yield"(%2) : (i32) -> ()
+
+// CHECK-NEXT: %[[CONST2:.+]] = arith.constant 1 : i32
+// CHECK-NEXT: %[[SUM:.+]] = arith.addi %[[CONST2]], %[[CONST2]] : i32
+// CHECK-NEXT: "foo.yield"(%[[SUM]]) : (i32) -> ()
+  }) : () -> ()
+
+  // CHECK: return %[[CONST1]] : i32
+  return %0 : i32
+}
Index: mlir/lib/Transforms/CSE.cpp
===
--- mlir/lib/Transforms/CSE.cpp
+++ mlir/lib/Transforms/CSE.cpp
@@ -15,6 +15,7 @@
 
 #include "mlir/IR/Dominance.h"
 #include "mlir/IR/PatternMatch.h"
+#include "mlir/Interfaces/CSEInterfaces.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Transforms/Passes.h"
@@ -61,7 +62,8 @@
 class CSEDriver {
 public:
   CSEDriver(RewriterBase &rewriter, DominanceInfo *domInfo)
-  : rewriter(rewriter), domInfo(domInfo) {}
+  : rewriter(rewriter), domInfo(domInfo),
+interfaces(rewriter.getContext()) {}
 
   /// Simplify all operations within the given op.
   void simplify(Operation *op, bool *changed = nullptr);
@@ -122,6 +124,9 @@
   DominanceInfo *domInfo = nullptr;
   MemEffectsCache memEffectsCache;
 
+  /// CSE interfaces in the present context that can modify CSE behavior.
+  DialectInterfaceCollection interfaces;
+
   // Va