[PATCH] D99977: [GVN] Properly invalidate ICF cache when we simplify a value

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2174
+      // For example, devirtualization to a willreturn function.
+      ICF->clear();
       Changed = true;
----------------
nikic wrote:
> It would be better to extend the IPT interface to allow more fine grained invalidation here. At least to only invalidate the blocks where the user is the current special instruction.
That seems reasonable, so something like: `IPT::clearUsersOf(Instruction*I)`? The intention is that this should be called before any RAUW operation, it will iterate the users, and if any of them are the cached special instruction, those blocks will be invalidated.


================
Comment at: llvm/test/Transforms/GVN/simplify-icf-cache-invalidation.ll:35
+  %tmp12 = bitcast %struct.zot* %arg to void (%struct.zot*, i1)***
+  %tmp13 = load void (%struct.zot*, i1)**, void (%struct.zot*, i1)*** %tmp12, align 8, !invariant.group !0
+  %tmp14 = getelementptr inbounds void (%struct.zot*, i1)*, void (%struct.zot*, i1)** %tmp13, i64 1
----------------
Please make this test case a bit more human readable. I'm guessing what happens here is that GVN somehow forwards the earlier invariant.group store to the invariant.group load, and simplifying the load instruction has the side effect of making the following indirect call "willreturn". There should be a way to test that without loads from null pointers and branches on undef, which make the test fragile to future GVN changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99977