MIR Statepoint refactoring. Part 3: Spill GC Ptr regs.

[dantrushin]

Summary

Extend FixupStatepointCallerSaved pass with ability to spill
statepoint GC pointer arguments (optionally allowing them on CSRs).
Special handling is required for invoke statepoints, because at MI
level single landing pad may be shared my multiple statepoints, so
we must ensure we spill landing pad's live-ins into the same stack
slots.

Full change set is available at D81603.

[skatkov]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

53 ↗

(On Diff #270107)

Add a comment that it is for debugging purpose. Please mention that not specifying this option means no restrictions at all.

127 ↗

(On Diff #270107)

SO.getNumDeoptArgsIdx() ?

529 ↗

(On Diff #270107)

If I understand correctly you are processing starting operands and they are defs.
so the condition MO.isDef() is actually exit condition of the loop (we processed all defs).
What else we can see as def? In other words what MO.isReg condition means?

618 ↗

(On Diff #270107)

don't you want to rephrase it as

unsigned LastStatepointwithRegsIdx = MaxStatepointsWithRegs.getNumOccurrences() ? MaxStatepointsWithRegs : Statepoints.size();

for (MachineInstr *I : Statepoints)
  Changed |= SPP.process(*I, ++NumStatepoints <= LastStatepointwithRegsIdx);

[skatkov]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

131 ↗

(On Diff #270107)

What if deopt args contains gc pointer?

328 ↗

(On Diff #270107)

in a separate function?

[dantrushin]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

53 ↗

(On Diff #270107)

Will do.

127 ↗

(On Diff #270107)

Yep.

131 ↗

(On Diff #270107)

At this point, we can not know.
And your code handles all deopt args uniformly already ;)
Mine adds some more restrictions

328 ↗

(On Diff #270107)

Ok

529 ↗

(On Diff #270107)

MO.isDef() asserts that MO.isReg(), so one always must check later before checking former

618 ↗

(On Diff #270107)

For upstream, maybe.
But since I have downstream changes here, I'd like to keep it that way if you do not mind.

[dantrushin]

Move copy propagation to separate function;
Simplify statepoint defs handling a bit;

[skatkov]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

55 ↗

(On Diff #274584)

Still do not see that not specifying this option actually means infinity.
Because it is a bit strange that
"Max number of statepoints allowed to pass GC Ptrs in registers" and default value 0.
So it makes me thinking that by default the behavior is off.

560 ↗

(On Diff #274584)

Please double check, that there is not problems with subregs here.

562 ↗

(On Diff #274584)

If you do not find reg in statepoint you continue to search. For example you found a Reg in previous statepoint, what does it mean?

To me it means that live gc pointer passed through next statepoint and is not mentioned in that statepoint. it is a bug.
I believe you should assert this or stop on the first statepoint.

Providing def as non-first statepoint seems to be a bug,

131 ↗

(On Diff #270107)

This is because I'm sure that GC pointer cannot be on register until your changes.

[skatkov]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

125 ↗

(On Diff #274584)

Do I understand correctly that with your changes ALL GC pointers must be defs?
So why do you need these iterations instead of just taking all defs?

401 ↗

(On Diff #274584)

what is the reason for this magic?

506 ↗

(On Diff #274584)

Don't you want to separate reload loads into separate function?
So we'll have:
spill registers
rewrite statepoint
insertReloads/unspill registers

[dantrushin]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

125 ↗

(On Diff #274584)

Strictly speaking, no. Only derived pointers passed in registers.
Are we guaranteed that all base pointers will appear as derived ones too?
If yes, then it is good catch, taking them from defs is simpler (but taking them from operand list instead of def list sounds a bit more natural, IMHO)

401 ↗

(On Diff #274584)

The reason is that TTI.loadRegFromStackSlot can insert load only before some existing instruction.

131 ↗

(On Diff #270107)

So nothing has changed.
At this point there is no way to detect deopt pointer which is not in gc list.
ISEL determines what pointers to pass where.
If implementation cannot handle pointer deopt value, not present in gc list, it should not enable it at all or spill all deopt values

[dantrushin]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

506 ↗

(On Diff #274584)

insertReloads uses local vector RegsToReload and MI (statepoint instruction).
To call insertReloads outside of rewriteStatepoint I will have to make that local vector and new statepoint instruction
available to insertReloads().

I don't think that making RegsToReload member variable or something like that:

SmallVector<Register, 8> RegsToReload;
SS.spillRegisters();
MachineInstr *NewStatepoint = SS.rewriteStatepoint(RegsToReload); // out parameter
SS.insertReloads(RegsToReload, NewStatepoint);

will be much cleaner.
But I can do that if you want.

[dantrushin]

Address review comments and fix clang-format errors

[skatkov]

A pretty close to be done.

Do we need a special tests for this patch?

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

125 ↗

(On Diff #274584)

I'm a bit confused here. What is the difference between derived and based pointer here?
You have an alive gc pointer. It might be relocated == can be changed. So it must be defined as def independent on whether it is a derived one or based one. Do I miss anything here?

If you are doubt and write that it works under assumption I would suggest under debug assert that sets collected by different way are the same.

401 ↗

(On Diff #274584)

Does it make sense to add an utility function to TTI which after some existing instruction? It looks more natural then this magic.

506 ↗

(On Diff #274584)

I do not have strong preference here. But separation seems to me makes sense.
At least rename the function rewriteStatepoint and definitely update the comment before the function due to it does additional things except rewriting statepoints.

[dantrushin]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

125 ↗

(On Diff #274584)

GC pointers always occurs in pairs (base, derived).
Only derived pointers are relocated (and so can be tied to defs of statepoint).
If base pointer need to be relocated it will appear as (base, base) pair.

It is not specified if the base pointer must be relocated together with its derived pointer.
At least, this is how I interpret LLVM docs. So I originally wrote it in a way I had no doubts of.

What's the point of having two implementation and comparing them with assert?
Assert is not proof that 'doubtful' implementation is correct.

401 ↗

(On Diff #274584)

I will have to implement that function in all backends and then spent 6 months or so asking people
to review change which they won't ever use...
I would like to avoid that

[skatkov]

I need an answer to the question about test and at least update doc for rewriteStatepoint.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

125 ↗

(On Diff #274584)

It will help you to catch easily an error in case I was wrong with defs.

401 ↗

(On Diff #274584)

ok

[dantrushin]

Return back old implementation of collectGCRegs as I've found case
where statepoint defs != gc args;

Refactor insertReloads() into a separate routine.

Will add some tests soon.

[dantrushin]

Add two simple MIR tests

[skatkov]

could you please explain the case due to you need to return back in collectGCRegs implementation?

[skatkov]

As I understand a lot of things here are done to workout the case that one catch block can correspond to different invoke instructions. Can you please add the test examine this case?

[dantrushin]

Add (hand crafted) test for shared landing pad;
Slightly change cache handling code to better handle shared landing pads;
Improve debug output;

[dantrushin]

Changed DerivedPtrMap and moved it to StatepointLowering.h as it only
needed for local gc.relocate processing;
Rebased on tip, rewrote lowerStatepointMetaArgs as been told;
Deleted test mods, will add separate test later;

[dantrushin]

Restore accidently destroyed review.

Change handling of shared landing pads - now we reserve spill slots
as we go, upon encountering first statepoint jumping to that landing pad.

Slightly modified tests to be less sensitive to unimportant changes
(spill ordering)

[skatkov]

Hi Denis, after you re-write the patch in this way, I guess we can do it clearly and simply in terms of reading.

Let's add set of reserved frame indices into FrameIndexesPerSize.

In reset method, you clean this new set.
In resetAndPreFill you just add all the reserved FI into this set.
In getFrameIndex you lookup FI in GlobalIndices and then take the first available FI which is not in Reserved set.

This way we can get away from this implicit assumptions that we reserved first n elements, supposing that they will processed first and so on...

What do you think?

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

261 ↗

(On Diff #277059)

It does not look valid to me. You'll set Line.Index to value corresponding to last processed pair from It.
While you need a max in this case...

611 ↗

(On Diff #277059)

remove redundant {} now.

[dantrushin]

Rework slot reservation according to comments.
Add an option to disable copy propagation.

[skatkov]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

276 ↗

(On Diff #278433)

assert that frame slot is reserved? (nobody used it)

327 ↗

(On Diff #278433)

Can we just track for each EhPad what registers were reloaded instead of this search?

400 ↗

(On Diff #278433)

I wonder, do we really need to collect GC regs. According to implementation, any register found in operand after some index is considered as GC reg.
So can we just compute this index and use it in this loop.

Do I miss anything?

[dantrushin]

Add tracking of reloads inserted in landing pads.

[dantrushin]

That EHPad reload tracking does not fit very well in existing design. I added it as a separate entity stored in StatepointProcessor and passed to
StatepointState::insertReloads.
Ideally, I would insert all reloads in a single sweep after all statepoints has been processed. But I did not find a clean way to do that without refactoring
(like merging StatepointStates functionality into`StatepointProcessor`).

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

400 ↗

(On Diff #278433)

This is to handle pointers found in both deopt state and gc pointer section uniformly.
To avoid case when same object is passed both in register and in stack slot.

[reames]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

551 ↗

(On Diff #278786)

There's an important semantic bug here. Test case and candidate fix can be found in https://reviews.llvm.org/D84964.

While writing the test case for this, I also stumbled across an unrelated functional bug. Filed as https://bugs.llvm.org/show_bug.cgi?id=46916

[skatkov]

Please fix the bug Philip mentioned and I think we are ready to land it.

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

130 ↗

(On Diff #278786)

In InstrEmitter I see getStatepointGCArgStartIdx which does exactly the same what you need.
It also contains:

// FIXME: need a better place for this. Put it in StackMaps?

Can you do it in a separate patch (w/o review) and just re-use this utility function here?

386 ↗

(On Diff #278786)

This overall algorithm is very sensitive to alone EHPad.
Looking into machine verifier, I see that in some cases we can have more than one landing pad.

 if (LandingPadSuccs.size() > 1 &&
  !(AsmInfo &&
    AsmInfo->getExceptionHandlingType() == ExceptionHandling::SjLj &&
    BB && isa<SwitchInst>(BB->getTerminator())) &&
  !isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn())))
report("MBB has more than one landing pad successor", MBB);

I would suggest to assert that here we have only one landing pad.

[dantrushin]

• Mark MMOs for gc register spill slots as load/store ones;
• Add assert that there is only one EHPad;

[dantrushin]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

130 ↗

(On Diff #278786)

No, it is slightly different.
During its lifetime statepoint meta args change their format few times. From bare %stack.0 form, to stackmap-encoded
version like 1, 8, %stack.0 here and down to frame register format.
This is the primary reason why I didn't put in into StackMaps.cpp - to remain generic, it'll need some kind of
parameter to tell which exact form it is parsing. Which would look ugly, IMHO

[skatkov]

Thanks!

[RKSimon]

llvm/test/CodeGen/X86/statepoint-fixup-copy-prop-neg.mir

3 ↗

(On Diff #285639)

@dantrushin This is failing on EXPENSIVE_CHECKS builds - please can you take a look?

[RKSimon]

llvm/test/CodeGen/X86/statepoint-fixup-copy-prop-neg.mir

3 ↗

(On Diff #285639)

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/8206

[dantrushin]

llvm/test/CodeGen/X86/statepoint-fixup-copy-prop-neg.mir

3 ↗

(On Diff #285639)

Thanks. I still have no email from bot, but reproduced it locally.
Will commit fix in a moment

[bjope]

llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp

224 ↗

(On Diff #285639)

Results in "unused variable" werror in noasserts build

475 ↗

(On Diff #285639)

Results in "unused variable" werror in noasserts build

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

if (RegisterSDNode *RN = dyn_cast<RegisterSDNode>(Node->getOperand(I - 1)))
         continue;
     NumImpUses = N - I;
     break;
   }
 
   return N;
 }
 
+/// Return starting index of GC operand list.
+// FIXME: need a better place for this. Put it in StackMaps?
+static unsigned getStatepointGCArgStartIdx(MachineInstr *MI) {
+  assert(MI->getOpcode() == TargetOpcode::STATEPOINT &&
+         "STATEPOINT node expected");
+  unsigned OperIdx = StatepointOpers(MI).getNumDeoptArgsIdx();
+  unsigned NumDeopts = MI->getOperand(OperIdx).getImm();
+  // At this point stack references has not been lowered yet, so they
+  // take single operand.
+  ++OperIdx;
+  while (NumDeopts--) {
+    MachineOperand &MO = MI->getOperand(OperIdx);
+    if (MO.isImm() && MO.getImm() == StackMaps::ConstantOp) {
+      ++OperIdx;
+      assert(MI->getOperand(OperIdx).isImm() &&
+             "Unexpected statepoint operand");
+    }
+    ++OperIdx;
+  }
+  return OperIdx;
+}
+
 /// EmitCopyFromReg - Generate machine code for an CopyFromReg node or an
 /// implicit physical register output.
 void InstrEmitter::
 EmitCopyFromReg(SDNode *Node, unsigned ResNo, bool IsClone, bool IsCloned,
                 Register SrcReg, DenseMap<SDValue, Register> &VRBaseMap) {
   Register VRBase;
   if (SrcReg.isVirtual()) {
     // Just use the input register directly!
void InstrEmitter::CreateVirtualRegisters(SDNode *Node,
                                        DenseMap<SDValue, Register> &VRBaseMap) {
   assert(Node->getMachineOpcode() != TargetOpcode::IMPLICIT_DEF &&
          "IMPLICIT_DEF should have been handled as a special case elsewhere!");
 
   unsigned NumResults = CountResults(Node);
   bool HasVRegVariadicDefs = !MF->getTarget().usesPhysRegsForValues() &&
                              II.isVariadic() && II.variadicOpsAreDefs();
   unsigned NumVRegs = HasVRegVariadicDefs ? NumResults : II.getNumDefs();
+  if (Node->getMachineOpcode() == TargetOpcode::STATEPOINT)
+    NumVRegs = NumResults;
   for (unsigned i = 0; i < NumVRegs; ++i) {
     // If the specific node value is only used by a CopyToReg and the dest reg
     // is a vreg in the same register class, use the CopyToReg'd destination
     // register instead of creating a new vreg.
     Register VRBase;
     const TargetRegisterClass *RC =
       TRI->getAllocatableClass(TII->getRegClass(II, i, TRI, *MF));
     // Always let the value type influence the used register class. The
if (Opc == TargetOpcode::STACKMAP || Opc == TargetOpcode::PATCHPOINT) {
     // convention. However, to simplify runtime support, they clobber the same
     // scratch registers as AnyRegCC.
     unsigned CC = CallingConv::AnyReg;
     if (Opc == TargetOpcode::PATCHPOINT) {
       CC = Node->getConstantOperandVal(PatchPointOpers::CCPos);
       NumDefs = NumResults;
     }
     ScratchRegs = TLI->getScratchRegisters((CallingConv::ID) CC);
+  } else if (Opc == TargetOpcode::STATEPOINT) {
+    NumDefs = NumResults;
   }
 
   unsigned NumImpUses = 0;
   unsigned NodeOperands =
     countOperands(Node, II.getNumOperands() - NumDefs, NumImpUses);
   bool HasVRegVariadicDefs = !MF->getTarget().usesPhysRegsForValues() &&
                              II.isVariadic() && II.variadicOpsAreDefs();
   bool HasPhysRegOuts = NumResults > NumDefs &&
for (SDNode *F = Node->getGluedUser(); F; F = F->getGluedUser()) {
         }
     }
   }
 
   // Finally mark unused registers as dead.
   if (!UsedRegs.empty() || II.getImplicitDefs() || II.hasOptionalDef())
     MIB->setPhysRegsDeadExcept(UsedRegs, *TRI);
 
+  // STATEPOINT is too 'dynamic' to have meaningful machine description.
+  // We have to manually tie operands.
+  if (Opc == TargetOpcode::STATEPOINT && NumDefs > 0) {
+    assert(!HasPhysRegOuts && "STATEPOINT mishandled");
+    MachineInstr *MI = MIB;
+    unsigned Def = 0;
+    unsigned Use = getStatepointGCArgStartIdx(MI) + 1;
+    while (Def < NumDefs) {
+      if (MI->getOperand(Use).isReg())
+        MI->tieOperands(Def++, Use);
+      Use += 2;
+    }
+  }
+
   // Run post-isel target hook to adjust this instruction if needed.
   if (II.hasPostISelHook())
     TLI->AdjustInstrPostInstrSelection(*MIB, Node);
 }
 
 /// EmitSpecialNode - Generate machine code for a target-independent node and
 /// needed dependencies.
 void InstrEmitter::

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

if (Register::isVirtualRegister(Reg))
     return;
 
   unsigned ResNo = User->getOperand(2).getResNo();
   if (Def->getOpcode() == ISD::CopyFromReg &&
       cast<RegisterSDNode>(Def->getOperand(1))->getReg() == Reg) {
     PhysReg = Reg;
   } else if (Def->isMachineOpcode()) {
     const MCInstrDesc &II = TII->get(Def->getMachineOpcode());
-    if (ResNo >= II.getNumDefs() &&
-        II.ImplicitDefs[ResNo - II.getNumDefs()] == Reg)
+    if (ResNo >= II.getNumDefs() && II.hasImplicitDefOfPhysReg(Reg))
       PhysReg = Reg;
   }
 
   if (PhysReg != 0) {
     const TargetRegisterClass *RC =
         TRI->getMinimalPhysRegClass(Reg, Def->getSimpleValueType(ResNo));
     Cost = RC->getCopyCost();
   }

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.h

public:
   }
 
   bool isStackSlotAllocated(int Offset) {
     assert(Offset >= 0 && Offset < (int)AllocatedStackSlots.size() &&
            "out of bounds");
     return AllocatedStackSlots.test(Offset);
   }
 
+  /// For each statepoint keep mapping from original derived pointer to
+  /// the statepoint node result defining its new value.
+  DenseMap<const Value *, SDValue> DerivedPtrMap;
+
 private:
   /// Maps pre-relocation value (gc pointer directly incoming into statepoint)
   /// into it's location (currently only stack slots)
   DenseMap<SDValue, SDValue> Locations;
 
   /// A boolean indicator for each slot listed in the FunctionInfo as to
   /// whether it has been used in the current statepoint.  Since we try to
   /// preserve stack slots across safepoints, there can be gaps in which

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

 STATISTIC(NumOfStatepoints, "Number of statepoint nodes encountered");
 STATISTIC(StatepointMaxSlotsRequired,
           "Maximum number of stack slots required for a singe statepoint");
 
 cl::opt<bool> UseRegistersForDeoptValues(
     "use-registers-for-deopt-values", cl::Hidden, cl::init(false),
     cl::desc("Allow using registers for non pointer deopt args"));
 
+cl::opt<bool> UseRegistersForGCPointers(
+    "use-registers-for-gc-values", cl::Hidden, cl::init(false),
+    cl::desc("Allow using registers for GC pointer meta args"));
+
 static void pushStackMapConstant(SmallVectorImpl<SDValue>& Ops,
                                  SelectionDAGBuilder &Builder, uint64_t Value) {
   SDLoc L = Builder.getCurSDLoc();
   Ops.push_back(Builder.DAG.getTargetConstant(StackMaps::ConstantOp, L,
                                               MVT::i64));
   Ops.push_back(Builder.DAG.getTargetConstant(Value, L, MVT::i64));
 }
 
static Optional<int> findPreviousSpillSlot(const Value *Val,
   // put handling of simple modifications in this function like it's done
   // for bitcasts we might end up reserving i's slot for 'i+1' because order in
   // which we visit values is unspecified.
 
   // Don't know any information about this instruction
   return None;
 }
 
-
 /// Return true if-and-only-if the given SDValue can be lowered as either a
 /// constant argument or a stack reference.  The key point is that the value
 /// doesn't need to be spilled or tracked as a vreg use.
 static bool willLowerDirectly(SDValue Incoming) {
   // We are making an unchecked assumption that the frame size <= 2^16 as that
   // is the largest offset which can be encoded in the stackmap format.
   if (isa<FrameIndexSDNode>(Incoming))
     return true;
 
   // The largest constant describeable in the StackMap format is 64 bits.
   // Potential Optimization:  Constants values are sign extended by consumer,
   // and thus there are many constants of static type > 64 bits whose value
   // happens to be sext(Con64) and could thus be lowered directly.
   if (Incoming.getValueType().getSizeInBits() > 64)
     return false;
 
   return (isa<ConstantSDNode>(Incoming) || isa<ConstantFPSDNode>(Incoming) ||
           Incoming.isUndef());
 }
 
-
 /// Try to find existing copies of the incoming values in stack slots used for
 /// statepoint spilling.  If we can find a spill slot for the incoming value,
 /// mark that slot as allocated, and reuse the same slot for this safepoint.
 /// This helps to avoid series of loads and stores that only serve to reshuffle
 /// values on the stack between calls.
 static void reservePreviousStackSlotForValue(const Value *IncomingValue,
                                              SelectionDAGBuilder &Builder) {
   SDValue Incoming = Builder.getValue(IncomingValue);
if (!Loc.getNode()) {
     auto PtrInfo = MachinePointerInfo::getFixedStack(MF, Index);
     auto *StoreMMO = MF.getMachineMemOperand(
         PtrInfo, MachineMemOperand::MOStore, MFI.getObjectSize(Index),
         MFI.getObjectAlign(Index));
     Chain = Builder.DAG.getStore(Chain, Builder.getCurSDLoc(), Incoming, Loc,
                                  StoreMMO);
 
     MMO = getMachineMemOperand(MF, *cast<FrameIndexSDNode>(Loc));
-    
     Builder.StatepointLowering.setLocation(Incoming, Loc);
   }
 
   assert(Loc.getNode());
   return std::make_tuple(Loc, Chain, MMO);
 }
 
 /// Lower a single value incoming to a statepoint node.  This value can be

 /// lowering is described in lowerIncomingStatepointValue.  This function is
 /// responsible for lowering everything in the right position and playing some
 /// tricks to avoid redundant stack manipulation where possible.  On
 /// completion, 'Ops' will contain ready to use operands for machine code
 /// statepoint. The chain nodes will have already been created and the DAG root
 /// will be set to the last value spilled (if any were).
 static void
 lowerStatepointMetaArgs(SmallVectorImpl<SDValue> &Ops,
-                        SmallVectorImpl<MachineMemOperand*> &MemRefs,                                    SelectionDAGBuilder::StatepointLoweringInfo &SI,
+                        SmallVectorImpl<MachineMemOperand *> &MemRefs,
+                        DenseMap<SDValue, int> &Ptr2ResNo,
+                        SelectionDAGBuilder::StatepointLoweringInfo &SI,
                         SelectionDAGBuilder &Builder) {
   // Lower the deopt and gc arguments for this statepoint.  Layout will be:
   // deopt argument length, deopt arguments.., gc arguments...
 #ifndef NDEBUG
   if (auto *GFI = Builder.GFI) {
     // Check that each of the gc pointer and bases we've gotten out of the
     // safepoint is something the strategy thinks might be a pointer (or vector
     // of pointers) into the GC heap.  This is basically just here to help catch
#endif
   // particular, if the callee spills callee preserved registers we may not
   // be able to find a value placed in that register during the call.  This is
   // fine for live-out, but not for live-through.  If we were willing to make
   // assumptions about the code generator producing the callee, we could
   // potentially allow live-through values in callee saved registers.
   const bool LiveInDeopt =
     SI.StatepointFlags & (uint64_t)StatepointFlags::DeoptLiveIn;
 
+  // Decide which deriver pointers will go on VRegs
+  const size_t MaxTiedRegs = 15; // Max  number of tied regs MI can have.
+  unsigned MaxVRegPtrs =
+      UseRegistersForGCPointers ? std::min(MaxTiedRegs, SI.Ptrs.size()) : 0;
+  // Use old spill scheme for cross-block relocates.
+  if (SI.StatepointInstr) {
+    const BasicBlock *BB = SI.StatepointInstr->getParent();
+    bool NonLocalReloc =
+        llvm::any_of(SI.GCRelocates, [BB](const GCRelocateInst *R) {
+          return R->getParent() != BB;
+        });
+    if (NonLocalReloc)
+      MaxVRegPtrs = 0;
+  }
+
+  LLVM_DEBUG(dbgs() << "Desiding how to lower GC Pointers:\n");
+  DenseSet<SDValue> LowerAsVReg;
+  for (const Value *P : SI.Ptrs) {
+    if (LowerAsVReg.size() == MaxVRegPtrs)
+      break;
+    SDValue PtrSD = Builder.getValue(P);
+    if (willLowerDirectly(PtrSD) || P->getType()->isVectorTy() ||
+        Builder.DL->isNonIntegralPointerType(P->getType())) {
+      LLVM_DEBUG(dbgs() << "spill "; PtrSD.dump(&Builder.DAG));
+      continue;
+    }
+    LLVM_DEBUG(dbgs() << "vreg "; PtrSD.dump(&Builder.DAG));
+    LowerAsVReg.insert(PtrSD);
+  }
+  LLVM_DEBUG(dbgs() << LowerAsVReg.size()
+                    << " derived pointers will go in vregs\n");
+
   auto isGCValue = [&](const Value *V) {
     auto *Ty = V->getType();
     if (!Ty->isPtrOrPtrVectorTy())
       return false;
     if (auto *GFI = Builder.GFI)
       if (auto IsManaged = GFI->getStrategy().isGCManagedPointer(Ty))
         return *IsManaged;
     return true; // conservative
   };
 
   auto requireSpillSlot = [&](const Value *V) {
-    return !(LiveInDeopt || UseRegistersForDeoptValues) || isGCValue(V);
+    if (isGCValue(V))
+      return LowerAsVReg.count(Builder.getValue(V)) == 0;
+    return !(LiveInDeopt || UseRegistersForDeoptValues);
   };
 
   // Before we actually start lowering (and allocating spill slots for values),
   // reserve any stack slots which we judge to be profitable to reuse for a
   // particular value.  This is purely an optimization over the code below and
   // doesn't change semantics at all.  It is important for performance that we
   // reserve slots for both deopt and gc values before lowering either.
   for (const Value *V : SI.DeoptState) {
     if (requireSpillSlot(V))
       reservePreviousStackSlotForValue(V, Builder);
   }
+
+  unsigned CurNumVRegs = 0;
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
+    SDValue SDV = Builder.getValue(SI.Ptrs[i]);
+    if (LowerAsVReg.count(SDV)) {
+      Ptr2ResNo[SDV] = (int)CurNumVRegs++;
+      continue;
+    }
+    Ptr2ResNo[SDV] = -1;
     reservePreviousStackSlotForValue(SI.Bases[i], Builder);
     reservePreviousStackSlotForValue(SI.Ptrs[i], Builder);
   }
+  assert(CurNumVRegs == LowerAsVReg.size() && "Lowered pointers mismatch");
 
   // First, prefix the list with the number of unique values to be
   // lowered.  Note that this is the number of *Values* not the
   // number of SDValues required to lower them.
   const int NumVMSArgs = SI.DeoptState.size();
   pushStackMapConstant(Ops, Builder, NumVMSArgs);
 
   // The vm state arguments are lowered in an opaque manner.  We do not know
   // what type of values are contained within.
+  LLVM_DEBUG(dbgs() << "Lowering deopt state\n");
   for (const Value *V : SI.DeoptState) {
     SDValue Incoming;
     // If this is a function argument at a static frame index, generate it as
     // the frame index.
     if (const Argument *Arg = dyn_cast<Argument>(V)) {
       int FI = Builder.FuncInfo.getArgumentFrameIndex(Arg);
       if (FI != INT_MAX)
         Incoming = Builder.DAG.getFrameIndex(FI, Builder.getFrameIndexTy());
     }
     if (!Incoming.getNode())
       Incoming = Builder.getValue(V);
+    LLVM_DEBUG(dbgs() << "Value " << *V
+                      << " requireSpillSlot = " << requireSpillSlot(V) << "\n");
     lowerIncomingStatepointValue(Incoming, requireSpillSlot(V), Ops, MemRefs,
                                  Builder);
   }
 
   // Finally, go ahead and lower all the gc arguments.  There's no prefixed
   // length for this one.  After lowering, we'll have the base and pointer
   // arrays interwoven with each (lowered) base pointer immediately followed by
   // it's (lowered) derived pointer.  i.e
   // (base[0], ptr[0], base[1], ptr[1], ...)
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
-    const Value *Base = SI.Bases[i];
-    lowerIncomingStatepointValue(Builder.getValue(Base),
-                                 /*RequireSpillSlot*/ true, Ops, MemRefs,
-                                 Builder);
+    SDValue Derived = Builder.getValue(SI.Ptrs[i]);
+    assert(Ptr2ResNo.find(Derived) != Ptr2ResNo.end() &&
+           "Broken DerivedPtr map");
 
-    const Value *Ptr = SI.Ptrs[i];
-    lowerIncomingStatepointValue(Builder.getValue(Ptr),
-                                 /*RequireSpillSlot*/ true, Ops, MemRefs,
+    bool RequireSpillSlot = (LowerAsVReg.count(Derived) == 0);
+    assert((RequireSpillSlot || Ptr2ResNo[Derived] >= 0) && "mismatch");
+    const Value *Base = SI.Bases[i];
+    lowerIncomingStatepointValue(Builder.getValue(Base), RequireSpillSlot, Ops,
+                                 MemRefs, Builder);
+    lowerIncomingStatepointValue(Derived, RequireSpillSlot, Ops, MemRefs,
                                  Builder);
   }
 
   // If there are any explicit spill slots passed to the statepoint, record
   // them, but otherwise do not do anything special.  These are user provided
   // allocas and give control over placement to the consumer.  In this case,
   // it is the contents of the slot which may get updated, not the pointer to
   // the alloca
#endif
   const Instruction *StatepointInstr = SI.StatepointInstr;
   auto &SpillMap = Builder.FuncInfo.StatepointSpillMaps[StatepointInstr];
 
   for (const GCRelocateInst *Relocate : SI.GCRelocates) {
     const Value *V = Relocate->getDerivedPtr();
     SDValue SDV = Builder.getValue(V);
     SDValue Loc = Builder.StatepointLowering.getLocation(SDV);
 
-    if (Loc.getNode()) {
+    if (Ptr2ResNo[SDV] >= 0) {
+      SpillMap[V] = None;
+    } else if (Loc.getNode()) {
       SpillMap[V] = cast<FrameIndexSDNode>(Loc)->getIndex();
     } else {
       // Record value as visited, but not spilled. This is case for allocas
       // and constants. For this values we can avoid emitting spill load while
       // visiting corresponding gc_relocate.
       // Actually we do not need to record them in this map at all.
       // We do this only to check that we are not relocating any unvisited
       // value.
SDValue SelectionDAGBuilder::LowerAsSTATEPOINT(
   // lower it, then reverse engineer the calling sequence.
 
   NumOfStatepoints++;
   // Clear state
   StatepointLowering.startNewStatepoint(*this);
   assert(SI.Bases.size() == SI.Ptrs.size() &&
          SI.Ptrs.size() <= SI.GCRelocates.size());
 
+  LLVM_DEBUG(dbgs() << "Lowering statepoint " << *SI.StatepointInstr << "\n");
 #ifndef NDEBUG
   for (auto *Reloc : SI.GCRelocates)
     if (Reloc->getParent() == SI.StatepointInstr->getParent())
       StatepointLowering.scheduleRelocCall(*Reloc);
 #endif
 
   // Lower statepoint vmstate and gcstate arguments
   SmallVector<SDValue, 10> LoweredMetaArgs;
   SmallVector<MachineMemOperand*, 16> MemRefs;
-  lowerStatepointMetaArgs(LoweredMetaArgs, MemRefs, SI, *this);
+  DenseMap<SDValue, int> Ptr2ResNo;
+  lowerStatepointMetaArgs(LoweredMetaArgs, MemRefs, Ptr2ResNo, SI, *this);
+
+  unsigned NumVRegs =
+      llvm::count_if(Ptr2ResNo, [](auto V) { return V.second >= 0; });
+  LLVM_DEBUG(dbgs() << "NumVRegs = " << NumVRegs << "\n");
 
   // Now that we've emitted the spills, we need to update the root so that the
   // call sequence is ordered correctly.
   SI.CLI.setChain(getRoot());
 
   // Get call node, we will replace it later with statepoint
   SDValue ReturnVal;
   SDNode *CallNode;
#endif
   Ops.push_back(Chain);
 
   // Same for the glue, but we add it only if original call had it
   if (Glue.getNode())
     Ops.push_back(Glue);
 
   // Compute return values.  Provide a glue output since we consume one as
   // input.  This allows someone else to chain off us as needed.
-  SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
+  SmallVector<EVT, 8> NodeTys;
+  for (auto &Ptr : SI.Ptrs) {
+    SDValue SD = getValue(Ptr);
+    if (Ptr2ResNo[SD] >= 0) {
+      NodeTys.push_back(SD.getValueType());
+    }
+  }
+  LLVM_DEBUG(dbgs() << "Statepoint has " << NodeTys.size() << " results\n");
+  assert(NodeTys.size() == NumVRegs && "Inconsistent GC Ptr lowering");
+  NodeTys.push_back(MVT::Other);
+  NodeTys.push_back(MVT::Glue);
 
+  unsigned NumResults = NodeTys.size();
   MachineSDNode *StatepointMCNode =
     DAG.getMachineNode(TargetOpcode::STATEPOINT, getCurSDLoc(), NodeTys, Ops);
   DAG.setNodeMemRefs(StatepointMCNode, MemRefs);
 
   SDNode *SinkNode = StatepointMCNode;
 
+  // Fill mapping from derived pointer to statepoint result denoting its
+  // relocated value.
+  auto &DPtrMap = StatepointLowering.DerivedPtrMap;
+  for (const auto *Relocate : SI.GCRelocates) {
+    Value *Derived = Relocate->getDerivedPtr();
+    SDValue SD = getValue(Derived);
+    if (Ptr2ResNo[SD] >= 0)
+      DPtrMap[Derived] = SDValue(StatepointMCNode, Ptr2ResNo[SD]);
+  }
+
   // Build the GC_TRANSITION_END node if necessary.
   //
   // See the comment above regarding GC_TRANSITION_START for the layout of
   // the operands to the GC_TRANSITION_END node.
   if (IsGCTransition) {
     SmallVector<SDValue, 8> TEOps;
 
     // Add chain
-    TEOps.push_back(SDValue(StatepointMCNode, 0));
+    TEOps.push_back(SDValue(StatepointMCNode, NumResults - 2));
 
     // Add GC transition arguments
     for (const Value *V : SI.GCTransitionArgs) {
       TEOps.push_back(getValue(V));
       if (V->getType()->isPointerTy())
         TEOps.push_back(DAG.getSrcValue(V));
     }
 
     // Add glue
-    TEOps.push_back(SDValue(StatepointMCNode, 1));
+    TEOps.push_back(SDValue(StatepointMCNode, NumResults - 1));
 
     SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
 
     SDValue GCTransitionStart =
         DAG.getNode(ISD::GC_TRANSITION_END, getCurSDLoc(), NodeTys, TEOps);
 
     SinkNode = GCTransitionStart.getNode();
   }
 
   // Replace original call
-  DAG.ReplaceAllUsesWith(CallNode, SinkNode); // This may update Root
+  // Call: ch,glue = CALL ...
+  // Statepoint: [gc relocates],ch,glue = STATEPOINT ...
+  unsigned NumSinkValues = SinkNode->getNumValues();
+  SDValue StatepointValues[2] = {SDValue(SinkNode, NumSinkValues - 2),
+                                 SDValue(SinkNode, NumSinkValues - 1)};
+  DAG.ReplaceAllUsesWith(CallNode, StatepointValues);
   // Remove original call node
   DAG.DeleteNode(CallNode);
 
   // DON'T set the root - under the assumption that it's already set past the
   // inserted node we created.
 
   // TODO: A better future implementation would be to emit a single variable
   // argument, variable return value STATEPOINT node here and then hookup the
#endif
 
   if (GCResult->getParent() == I.getParent()) {
     // Result value will be used in a same basic block. Don't export it or
     // perform any explicit register copies. The gc_result will simply grab
     // this value. 
     setValue(&I, ReturnValue);
     return;
   }
-  
   // Result value will be used in a different basic block so we need to export
   // it now.  Default exporting mechanism will not work here because statepoint
   // call has a different type than the actual call. It means that by default
   // llvm will create export register of the wrong type (always i32 in our
   // case). So instead we need to create export register with correct type
   // manually.
   // TODO: To eliminate this problem we can remove gc.result intrinsics
   //       completely and make statepoint call to return a tuple.
#endif
 
   if (SD.isUndef() && SD.getValueType().getSizeInBits() <= 64) {
     // Lowering relocate(undef) as arbitrary constant. Current constant value
     // is chosen such that it's unlikely to be a valid pointer.
     setValue(&Relocate, DAG.getTargetConstant(0xFEFEFEFE, SDLoc(SD), MVT::i64));
     return;
   }
 
+  auto &DPtrMap = StatepointLowering.DerivedPtrMap;
+  auto It = DPtrMap.find(DerivedPtr);
+  if (It != DPtrMap.end()) {
+    setValue(&Relocate, It->second);
+    return;
+  }
+
   auto &SpillMap = FuncInfo.StatepointSpillMaps[Relocate.getStatepoint()];
   auto SlotIt = SpillMap.find(DerivedPtr);
   assert(SlotIt != SpillMap.end() && "Relocating not lowered gc value");
   Optional<int> DerivedPtrLocation = SlotIt->second;
 
   // We didn't need to spill these special cases (constants and allocas).
   // See the handling in spillIncomingValueForStatepoint for detail.
   if (!DerivedPtrLocation) {

llvm/lib/CodeGen/TargetLoweringBase.cpp

if (!llvm::any_of(MI->operands(),
                     [](MachineOperand &Operand) { return Operand.isFI(); }))
     return MBB;
 
   MachineInstrBuilder MIB = BuildMI(MF, MI->getDebugLoc(), MI->getDesc());
 
   // Inherit previous memory operands.
   MIB.cloneMemRefs(*MI);
 
-  for (auto &MO : MI->operands()) {
+  for (unsigned i = 0; i < MI->getNumOperands(); ++i) {
+    MachineOperand &MO = MI->getOperand(i);
     if (!MO.isFI()) {
+      unsigned TiedTo = i;
+      if (MO.isReg() && MO.isTied())
+        TiedTo = MI->findTiedOperandIdx(i);
       MIB.add(MO);
+      if (TiedTo < i)
+        MIB->tieOperands(TiedTo, MIB->getNumOperands() - 1);
       continue;
     }
 
     // foldMemoryOperand builds a new MI after replacing a single FI operand
     // with the canonical set of five x86 addressing-mode operands.
     int FI = MO.getIndex();
 
     // Add frame index operands recognized by stackmaps.cpp