[PATCH] D141712: [GVN] Improve PRE on load instructions

Thu Jan 26 12:32:37 PST 2023

Carrot added a comment.

In D141712#4082217 <https://reviews.llvm.org/D141712#4082217>, @nikic wrote:

> It looks like the new version of the patch ended up being more expensive in terms of compile-time: Original <http://llvm-compile-time-tracker.com/compare.php?from=fe2ca62e92c82efcbbd10916e352bdfeddb80e19&to=1d81778eee4d4c38db40ec50a5e18ac76e251b44&stat=instructions:u>, New <http://llvm-compile-time-tracker.com/compare.php?from=287508cd9c4396c8845d92310d258879202a179e&to=5f1448fe1585b5677d5f0064e4eeac3b493d8a18&stat=instructions%3Au>
>
> I wonder whether this is because not removing the load essentially always forces an extra GVN iteration?

I think so, it causes all triggering cases need another iteration.

================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1462
+        if (uint32_t ValNo = VN.lookup(OldLoad, false))
+          removeFromLeaderTable(ValNo, OldLoad, OldLoad->getParent());
+        // To avoid deleting an instruction from different BB, we just leave
----------------
nikic wrote:
> If we're not removing the load, we probably shouldn't be removing it from the leader table either?
Because we expect it to be deleted in the next iteration, and the same value is also available in the NewLoad instruction, so I think it should not be used by other optimizations, and assume it's not available in the leader table.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141712