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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 5 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/IR/Builders.h:465
+  /// rewrite patterns.
+  virtual void eraseOp(Operation *op) { op->erase(); }
+  virtual ~OpBuilder() {}
----------------
ftynse wrote:
> 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.
reverted this for now, it can be done separately.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2567
+
+  newYield.erase();
+  return newLoop;
----------------
ftynse wrote:
> 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. 
For now I reverted to a comment, when I need it in a PatternRewriter I'll update.
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80656