[PATCH] D76305: [RS4GC] Fix algorithm to avoid setting vector BDV for scalar derived pointer

[Anna Thomas via Phabricator via llvm-commits]

anna marked 2 inline comments as done.
anna added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:641
 /// is it known to be a base pointer?  Or do we need to continue searching.
-static bool isKnownBaseResult(Value *V) {
+static bool isKnownBaseResult(Value *V, Value *Input) {
   if (!isa<PHINode>(V) && !isa<SelectInst>(V) &&
----------------
dantrushin wrote:
> Is `Input` a derived pointer here for which we check if `V` is a base of?.. Or vice versa?
It's actually overloaded to answer two questions:
1. is V a knownBase?
2. Is V and Input having differing types (Input can be either the derivedPtr where V is the BasePtr or it can be the base pointer for V)
Technically, in most cases, we don't need `Input` because `isKnownBaseResult` is just depending on whether `V` is a knownBase.

I think a better factoring of the code will be to have a helper function which utilizes `isKnownBaseResult`  and *then* checks whether Base and Input are both vector or both scalar type.

That will be cleaner and avoid the confusion below.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:884
       Value *BDV = Pair.first;
-      assert(!isKnownBaseResult(BDV) && "why did it get added?");
+      assert(!isKnownBaseResult(BDV, Pair.second.getBaseValue()) &&
+             "why did it get added?");
----------------
dantrushin wrote:
> Here I am confused.
> Isn't `Pair.second.getBaseValue()` supposed to be base value for `BDV`? And we're asking the other way?
> Same question for lines 944, 977 and few more below.
Yes, Pair.second.getBaseValue() is supposed to be base value for BDV. 
This is actually just a check for the types to make sure we don't fail the assert here.  
I think a cleaner assert is this:
`assert((!isKnownBaseResult(BDV) || (BDV and Pair.second.getBaseValue() have differing scalar/vector types)) && "why did it get added")`

Basically, knownBases that have different scalar/vector type from the 

The lines you're referring to below will also be handled in similar way.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76305