[PATCH] D80656: [mlir][SCF] Add utility to clone an scf.ForOp while appending new yield values.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/IR/Builders.h:465
+  /// rewrite patterns.
+  virtual void eraseOp(Operation *op) { op->erase(); }
+  virtual ~OpBuilder() {}
----------------
OpBuilder was refactored to use the Listener pattern instead of virtual methods. If we agree that we want to have `eraseOp` on `OpBuilder`, it should also go through the listener.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:323
 
 #endif // MLIR_TRANSFORMS_LOOP_UTILS_H
----------------
My commit-level comment might have gone unnoticed, so reiterating here: "It feels like this should live in SCF rather than LoopUtils, it's not anyhow generic."


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2567
+
+  newYield.erase();
+  return newLoop;
----------------
nicolasvasilache wrote:
> bondhugula wrote:
> > ftynse wrote:
> > > This is not allowed if OpBuilder is a ConversionRewriter, use b.eraseOp instead
> > Hmm... but there is no OpBuilder::eraseOp; it only exists in PatternRewriters. 
> added one, let's see what @rriddle says
Good point, @bondhugula !

@nicolasvasilache do you intend to use this in a place where no PatternRewriter is available? If not, just take a PatternRewriter& instead of OpBuilder& and be done with it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80656