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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

Patch looks good to me. Let's wait if anyone else has any comments.



================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:695
+  if (!MemLocOpt)
+    // "target" intrinsic forms of loads aren't currently known to
+    // MemoryLocation::get.  TODO
----------------
I couldn't parse this comment.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:803
+    if (match(Inst, m_Intrinsic<Intrinsic::invariant_start>())) {
+      if (!Inst->use_empty())
+        continue;
----------------
Pls add a comment here that this handles invariant end. So, `AvailableInvariants` are guaranteed to be invariant indefinitely from `CurrentGeneration` onwards.


================
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
----------------
why are we not able to catch this case? `invariant.load` is more powerful than invariant.start and we unconditionally return true for `isOperatingOnInvariantMemAt`


================
Comment at: test/Transforms/EarlyCSE/invariant.start.ll:257
+
+define i32 @test_false_negative_scope(i32* %p) {
+; CHECK-LABEL: @test_false_negative_scope
----------------
Any idea if MemorySSA will catch this case?


Repository:
  rL LLVM

https://reviews.llvm.org/D43716