[PATCH] D148109: [mlir] Add a generic mem2reg implementation.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: mlir/lib/Transforms/Mem2Reg.cpp:245-246
+    for (OpOperand *blockingUse : newBlockingUses) {
+      assert(llvm::find(user->getResults(), blockingUse->get()) !=
+             user->result_end());
+
----------------
nit: Use `llvm::is_contained`


================
Comment at: mlir/lib/Transforms/Mem2Reg.cpp:188
+
+  assert(isResultOrNewBlockArgument &&
+         "a slot must be a result of the allocator or an argument of the "
----------------
gysit wrote:
> kuhar wrote:
> > gysit wrote:
> > > This may trigger a warning if compiled without assertion. The standard way to avoid this is putting 
> > > 
> > > ```
> > > (void)isResultOrNewBlockArgument;
> > > ```
> > > before the assertion. Also consider starting the search from the slot.ptr and then call get defining op / cast it to a block argument, e.g:
> > > ```
> > > auto isResultOrBlockArgument = [&](Value value) {
> > >   if (value.getDefiningOp() == allocator)
> > >     return true;
> > >   if (BlockArgument arg = value.dyn_cast<BlockArgument>())
> > >     if (arg.getOwner()->getParentOp() == allocator)
> > >       return true;
> > >   return false;
> > > }
> > > ```
> > > 
> > In this specific case, shouldn't the whole body of this constructor be guarded by `#ifndef NDEBUG`?
> > 
> > IE this block does not produce any results/side-effects used in non-debug builds.
> Yes that sounds right. Without NDEBUG there is probably an unused variable warning as well.
This hasn't been fully addressed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148109