[PATCH] D43716: [EarlyCSE] Exploit open ended invariant.start scopes

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:695
+  if (!MemLocOpt)
+    // "target" intrinsic forms of loads aren't currently known to
+    // MemoryLocation::get.  TODO
----------------
anna wrote:
> I couldn't parse this comment.
There are a family of intrinsics which were partially taught to early CSE "as if" they were real loads and stores.  This code doesn't handle them yet because it was added as a layer on tp of MemLoc, not part of the MemLoc routines themselves.  I have no plans to handle them.


================
Comment at: test/Transforms/EarlyCSE/invariant-loads.ll:99
+define void @test_false_negative_dse(i32* %p, i1 %cnd) {
+; CHECK-LABEL: @test_false_negative_dse
+; CHECK: store
----------------
anna wrote:
> why are we not able to catch this case? `invariant.load` is more powerful than invariant.start and we unconditionally return true for `isOperatingOnInvariantMemAt`
The way this works currently is that we're only creating invariant scopes for invariant.start calls.  I agree we could (and in the near future will) create them for invariant.load, but at the moment we're not.  Given that, the call advances the generation.  The unconditional "yes" for invariant_loads isn't triggered since we never pass in the load.  Instead, we see the store is not an invariant_load and fallback to the conservative logic.  


================
Comment at: test/Transforms/EarlyCSE/invariant.start.ll:257
+
+define i32 @test_false_negative_scope(i32* %p) {
+; CHECK-LABEL: @test_false_negative_scope
----------------
anna wrote:
> Any idea if MemorySSA will catch this case?
At the moment, MemorySSA has no knowledge of invariant.start, so no.  I do plan on adding support, but I'm going to start with open ended scopes.


https://reviews.llvm.org/D43716