[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:188
+
+  assert(isResultOrNewBlockArgument &&
+         "a slot must be a result of the allocator or an argument of the "
----------------
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.


================
Comment at: mlir/lib/Transforms/Mem2Reg.cpp:423-424
+  llvm::SetVector<Operation *> usersToRemoveUses;
+  for (auto &[user, _] : userToBlockingUses)
+    usersToRemoveUses.insert(user);
+  SetVector<Operation *> sortedUsersToRemoveUses =
----------------
nit: `for (auto &user: llvm::make_first_range(userToBlockingUses))`?


================
Comment at: mlir/lib/Transforms/Mem2Reg.cpp:527
+
+struct Mem2Reg : public impl::Mem2RegBase<Mem2Reg> {
+  void runOnOperation() override {
----------------
nit: `public` is the default for `struct` and can be omitted here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148109