[mlir] Remove `Transforms/SideEffectUtils.h` and move the methods into `Interface/SideEffectInterfaces.h`.

[mravishankar]

Summary

The methods in SideEffectUtils.h (and their implementations in
SideEffectUtils.cpp) seem to have similar intent to methods already
existing in SideEffectInterfaces.h. Move the decleration (and
implementation) from SideEffectUtils.h (and SideEffectUtils.cpp)
into SideEffectInterfaces.h (and SideEffectInterface.cpp).

Also drop the SideEffectInterface::hasNoEffect method in favor of
mlir::isMemoryEffectFree which actually recurses into the operation
instead of just relying on the hasRecursiveMemoryEffectTrait
exclusively.

[mravishankar]

Drop use of hasNoEffect and use isMemoryEffectFree.

[rriddle]

mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp

348–357

What is the relation with these changes?

[mravishankar]

mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp

348–357

Good catch. This was something I hit while debugging before the better solution was suggest by @Mogball on discord. Ill revert this.

[mravishankar]

Address comments.

[mravishankar]

Rebase

[sanjoy]

Since hasNoEffect did not walk the op regions and isMemoryEffectFree does, I think with this change we have a couple of redundant checks on the attached regions.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

661 ↗

(On Diff #475272)

Do we still need the clone->getNumRegions() != 0 bit, now that isMemoryEffectFree recursively checks the regions? Same comment elsewhere.

mlir/lib/Interfaces/SideEffectInterfaces.cpp

160 ↗

(On Diff #475272)

These comments need to be adjusted -- it should no longer say "it cannot be moved". Same for code comments below.

[mravishankar]

In D137857#3926091, @sanjoy wrote:

Since hasNoEffect did not walk the op regions and isMemoryEffectFree does, I think with this change we have a couple of redundant checks on the attached regions.

I am not sure I follow. It seems like dropping the checks you mentioned increases the scope of analysis past what is intended with this change. I think those need to be follow ups (by folks who understand that code which isnt me) . I hope I can land this change without this ballooning much more.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

661 ↗

(On Diff #475272)

I think that is outside the scope of this change. That seems like it can have non-trivial changes to the intent of the code here that I dont fully understand. It maybe a follow up for people who are currently working in this area.

mlir/lib/Interfaces/SideEffectInterfaces.cpp

160 ↗

(On Diff #475272)

This is just a move of the code. So the comment is what was there earlier. Can you suggest what you want here?

[mravishankar]

Rebase.

[sanjoy]

mlir/lib/Interfaces/SideEffectInterfaces.cpp

173 ↗

(On Diff #475293)

Maybe say "// Check if any operations in the inner regions have memory effects" or equivalent.

160 ↗

(On Diff #475272)

You could just delete the comment probably since saying "If the op has no memory effects then it is memory effect free" is not very useful :)

[mravishankar]

Maybe fix lit test failure on Windows.

[mravishankar]

Lit test fixes

[mravishankar]

FYI, I landed this even though @sanjoy hasnt accepted it. I had a chat with him offline earlier where he was OK with making some of the more functional/comment changes after the fact.

mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td

bool hasNoEffect() {
       getEffects(effects);
       return effects.empty();
     }
 
     /// Returns true if the given operation has no effects for this interface.
     static bool hasNoEffect(Operation *op) {
       if (auto interface = dyn_cast<}] # name # [{>(op))
         return interface.hasNoEffect();
-      return op->hasTrait<::mlir::OpTrait::HasRecursiveMemoryEffects>();
+      return !op->hasTrait<::mlir::OpTrait::HasRecursiveMemoryEffects>();
     }
 
     /// Collect all of the effect instances that operate on the provided value
     /// and place them in 'effects'.
     void getEffectsOnValue(::mlir::Value value,
               llvm::SmallVectorImpl<::mlir::SideEffects::EffectInstance<
               }] # baseEffect # [{>> & effects) {
       getEffects(effects);

mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp

void operator()(async::ExecuteOp executeOp) {
     }
   }
 };
 
 // Replaces synchronous GPU ops in the op's region with asynchronous ones and
 // inserts the necessary synchronization (as gpu.wait ops). Assumes sequential
 // execution semantics and that no GPU ops are asynchronous yet.
 void GpuAsyncRegionPass::runOnOperation() {
-  if (getOperation()->walk(ThreadTokenCallback(getContext())).wasInterrupted())
+  auto operation = getOperation();
+  if (operation->walk(ThreadTokenCallback(getContext())).wasInterrupted())
     return signalPassFailure();
 
   // Collect gpu.wait ops that we can move out of async.execute regions.
-  getOperation().getRegion().walk(DeferWaitCallback());
-  // Makes each !gpu.async.token returned from async.execute op have single use.
-  getOperation().getRegion().walk(SingleTokenUseCallback());
+  if (operation->getNumRegions() != 0) {
+    Region &region = operation.getRegion();
+    if (!region.empty()) {
+      region.walk(DeferWaitCallback());
+      // Makes each !gpu.async.token returned from async.execute op have single
+      // use.
+      region.walk(SingleTokenUseCallback());
+    }
+  }
 }
 
 std::unique_ptr<OperationPass<func::FuncOp>> mlir::createGpuAsyncRegionPass() {
   return std::make_unique<GpuAsyncRegionPass>();
 }