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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2191
+          std::make_tuple(GCR->getBasePtrIndex(), GCR->getDerivedPtrIndex(),
+                          (unsigned)GCR->isTiedToLandingPad());
+      auto *&Dup = IdxToRel[Key];
----------------
reames wrote:
> 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.  
I'll consider but not sure whether tuple has function support token type.
I cannot be based on isTiedToInvoke property because I need to separate gc relocation from the same statepoint for those which are in landing pad and in normal path. Both these relocations will have the same true value of isTiedToInvoke property.

So I will try token directly and if it does not work I will stay with the current approach.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2197
+        else
+          assert(DT.dominates(Dup, GCR) && "Domination failed");
+        replaceInstUsesWith(*GCR, Dup);
----------------
reames wrote:
> This assert will fail.  Consider:
> 
> %token = call statepoint....
> if (c)
>   %gcr = relocate(token, 0, 0)
> else
>   %gcr2 = relocate(token, 0, 0)
Ok, I'll cover this case, it will do an algorithm a bit more complex :)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2252
 
+      if (CheckRelocationForDuplication(&GCR))
+        continue;
----------------
reames wrote:
> You haven't changed what is live, so the early continue is pointless.
If I found a duplicate which has already been processed that it means that both derived and base pointers are already is in LiveGCValues. I can add an assert.


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

https://reviews.llvm.org/D97837