[PATCH] D77036: [CodeGen] Fix isGCValue utility for statepoint lowering

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked 2 inline comments as done.
skatkov added a comment.

In D77036#1950340 <https://reviews.llvm.org/D77036#1950340>, @reames wrote:

> Er, I think the specification simplify needs changed here.  As we've discussed offline, it's impossible to enforce having a gc pointer in the deopt list always be in the gc list (in any form).  This was a flaw in the specification from the begging and we should fix the docs, not try to hack up the code to preserve it a bit longer.
>
> The key example of why this can't work:
>  %p1 = bitcast i8* %p to i8*
>  statepoint [gc = (%p1)], [deopt = (%p1)]
>
> The optimizer is allowed to replace either use (or both) of %p1 with %p.  If it updates only one of the two (entirely legal), the two sets do not overlap.
>
> If we need to enforce a property for GC values within deopt list, we should directly use the type of the pointer in question.  Anything else appears unsound.


ok, then I will prepare a patch which eliminates the questionable requirement from spec.

In terms of fixing this one I see two ways to resolve it:

1. Use the same isHandledGCPointerType utility as RewriteStatepointsForGC for detection of GC pointer
2. Conservatively suggest any pointer to be a GC pointer.

What way do you prefer?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:497
   }
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
     reservePreviousStackSlotForValue(SI.Bases[i], Builder);
----------------
dantrushin wrote:
> Is it correct that we process `SI.Bases` and `SI.Ptrs` here, not `SI.GCArgs`?
It is fine due to it is only optimization and is not required for correctness.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:530
   // (base[0], ptr[0], base[1], ptr[1], ...)
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
     const Value *Base = SI.Bases[i];
----------------
dantrushin wrote:
> And here we lower not all GCArgs, but relocated pointers only....
That is also fine. Bases and Ptrs are build from GCArgs by elimination of the same values, so it is ok to lower only unique values.


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

https://reviews.llvm.org/D77036