[PATCH] D97837: [InstCombine] Remove gc.relocate duplicates

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2191
+          std::make_tuple(GCR->getBasePtrIndex(), GCR->getDerivedPtrIndex(),
+                          (unsigned)GCR->isTiedToLandingPad());
+      auto *&Dup = IdxToRel[Key];
----------------
You can avoid the isTiedToLandingPad routine in a couple of ways:
- Key directly off the token value
- Key off the existing isTiedToInvoke property

I'm also fine with you adding the cover function if desired, just pointing out alternatives in case you hadn't considered them.  


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2197
+        else
+          assert(DT.dominates(Dup, GCR) && "Domination failed");
+        replaceInstUsesWith(*GCR, Dup);
----------------
This assert will fail.  Consider:

%token = call statepoint....
if (c)
  %gcr = relocate(token, 0, 0)
else
  %gcr2 = relocate(token, 0, 0)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2252
 
+      if (CheckRelocationForDuplication(&GCR))
+        continue;
----------------
You haven't changed what is live, so the early continue is pointless.


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

https://reviews.llvm.org/D97837