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

[Tobias Gysi via Phabricator via llvm-commits]

gysit added inline comments.


================
Comment at: mlir/lib/Transforms/Mem2Reg.cpp:188
+
+  assert(isResultOrNewBlockArgument &&
+         "a slot must be a result of the allocator or an argument of the "
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148109