[PATCH] D118572: [NewGVN] Improve phi-of-ops fix to allow loads that loop invariant-ish

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thank you for looking at this! Please see inline.



================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:2601
+  if (OrigI->mayReadFromMemory()) {
+    MemoryAccess *OriginalAccess = getMemoryAccess(OrigI);
+    MemoryAccess *DefiningAccess =
----------------
```
if (!OriginalAccess)
  return true;
```
(if instruction is not modeled in MSSA)


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:2612
+    if (OriginalAccess->getBlock() != DefiningAccess->getBlock() &&
+        MSSA->dominates(DefiningAccess, OriginalAccess))
+      return true;
----------------
A clobbering access (`DefiningAccess`) always dominates the queried access (`OriginalAccess`), so the second condition is redundant.
Now, is the first condition (different BBs) enough? I don't think it is. The store can still be inside the loop but in an earlier block.

```
BBa (store)  -> BBb -> BBd (load) -
    ^        \-> BBc /^          |
    |____________________________|
```
Inserting the phi-of-ops in BBa is incorrect, if the load will use the value from phi-of-ops instead of reloading. This example is what I was seeing in one of the bug reports (one load was in BBa and it's covered by the tests here, another was in BBd.

I think the condition here is something like `if (DT->properlyDominates(DefiningAccess->getBlock(), PHIBlock))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118572