[PATCH] D118572: [NewGVN] Improve phi-of-ops fix to allow loads that loop invariant-ish
Alina Sbirlea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 25 15:12:30 PST 2022
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
More information about the llvm-commits
mailing list