[PATCH] D118676: [RS4GC] Extract rematerilazable candidate search. NFC.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

General direction seems reasonable, a couple inline comments, but likely to converge to an LGTM quickly.

However, I'd suggest taking a step back and asking whether this code makes sense at all.  I don't want to make this blocking, but I think it does make sense to reexamine before going much further.

Given a random derived pointer P and it's base B, we can always find the offset via sub(ptrtoint(P), ptrtoint(B)).  Materializing the ptrtoint is maybe undesirable, but we already do this in a couple places in the existing code.  (Remember, this is a lowering pass.)  See gc.get.pointer.offset.

I'd argue this means we should either be remating everything, or nothing.  The current heuristic - which amounts to "can we find the gep chain?" - really doesn't seem defensible.



================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:287
+  SmallVector<Instruction *, 3> ChainToBase;
+  // Original base.
+  Value *RootOfChain;
----------------
Shouldn't this always be the base pointer of the last element in the chain?  If so, might be good to either not store explicitly, or at least assert.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2275
       // deficiency in the findBasePointer algorithm.
       if (!AreEquivalentPhiNodes(*OrigRootPhi, *AlternateRootPhi))
         continue;
----------------
This really looks like something we should fix in the findBasePointer code, but we can leave it for the moment.  We're adding complexity to avoid fixing another problem, and that seems non ideal. 


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2330
+    // For each live pointer find get its defining chain.
+    SmallVector<Instruction *, 3> ChainToBase = Record.ChainToBase;
+    // Walk backwards to visit top-most instructions first.
----------------
You're doing a copy here, but I don't see either the copy or the source modified.  Did you mean this to be a const reference?


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

https://reviews.llvm.org/D118676