This is an archive of the discontinued LLVM Phabricator instance.

[Statepoints] Relocating vectors of pointers
ClosedPublic

Authored by reames on Dec 17 2015, 5:14 PM.

Details

Summary

Currently, we try to split vectors of pointers back into their component pointer elements during rewrite-statepoints-for-gc. This is less than ideal since presumably the vectorizer chose to vectorize for a reason. :) It's also been a source of bugs - in particular, the relocation logic as currently implemented was recently discovered to be wrong.

The alternate approach is to allow gc.relocates of vector-of-pointer type and update the backend to handle them. That's what this patch tries to do. This won't actually enable vector-of-pointers in practice - there are some RS4GC changes needed - but the lowering is standalone and testable so it makes sense to separate.

Note that there are some known cases around vector constants which this patch does not handle. Once this is in, I'll send another patch with test cases. I decided to separate them since it would let me make progress on other parts - RS4GC, frontend changes - in parallel. The additional changes required don't look particularly complex.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 43199.Dec 17 2015, 5:14 PM
reames retitled this revision from to [GC][In Progress] Relocating vectors of pointers.
reames updated this object.
reames added a subscriber: llvm-commits.
sanjoy edited edge metadata.Dec 18 2015, 12:06 AM

As I've said in person, I'm worried about LLVM (SelectionDAG & later,
specifically) making assumptions on how spill slots can be used. For
instance, if there is an assumption that spill slots are strictly
frame local, then this transform becomes legal (gc_ref is a callee
saved register):

[%rsp + 48] = gc_ref
call _foo ## statepoint
gc_ref_relocated = [%rsp + 48]

>

[%rsp + 48] = gc_ref
gc_ref_relocated = [%rsp + 48]
call _foo ## statepoint

Since the call to _foo could not have possibly clobbered a frame local
location (since the frame is where you spill registers to *avoid* them
getting clobbered!).

In fact, I suspect this is unsound even today. I still can't see the
whole picture, but for instance, in MachineLICM.cpp you have:

static bool InstructionStoresToFI(const MachineInstr *MI, int FI) {
  for (MachineInstr::mmo_iterator o = MI->memoperands_begin(),
         oe = MI->memoperands_end(); o != oe; ++o) {
    if (!(*o)->isStore() || !(*o)->getPseudoValue())
      continue;
    if (const FixedStackPseudoSourceValue *Value =
        dyn_cast<FixedStackPseudoSourceValue>((*o)->getPseudoValue())) {
      if (Value->getFrameIndex() == FI)
        return true;
    }
  }
  return false;
}

and I don't see how a STATEPOINT node (as created in
StatepointLowering.cpp) gets a FixedStackPseudoSourceValue in its
memoperands vector (the loads and stores we generate around the
STATEPOINT node the right aliasing information).

As I've said in person, I'm worried about LLVM (SelectionDAG & later,
specifically) making assumptions on how spill slots can be used.

The general concern is a valid one. I think I've talked myself/you've talked me around to an alternate approach. If we add new flags to StackObject called 'isDeoptSpil' and 'isGCSpill' we can customize the behaviour much more obviously and safely. I'm going to restructure this patch in that direction unless someone speaks up to say that's a bad idea.

For
instance, if there is an assumption that spill slots are strictly
frame local, then this transform becomes legal (gc_ref is a callee
saved register):

[%rsp + 48] = gc_ref
call _foo ## statepoint
gc_ref_relocated = [%rsp + 48]

>

[%rsp + 48] = gc_ref
gc_ref_relocated = [%rsp + 48]
call _foo ## statepoint

Since the call to _foo could not have possibly clobbered a frame local
location (since the frame is where you spill registers to *avoid* them
getting clobbered!).

This is supposed to be handled by the volatile, load, and store flags we put on the memory operands in emitPatchPoint, but see below....

In fact, I suspect this is unsound even today. I still can't see the
whole picture, but for instance, in MachineLICM.cpp you have:

static bool InstructionStoresToFI(const MachineInstr *MI, int FI) {
  for (MachineInstr::mmo_iterator o = MI->memoperands_begin(),
         oe = MI->memoperands_end(); o != oe; ++o) {
    if (!(*o)->isStore() || !(*o)->getPseudoValue())
      continue;
    if (const FixedStackPseudoSourceValue *Value =
        dyn_cast<FixedStackPseudoSourceValue>((*o)->getPseudoValue())) {
      if (Value->getFrameIndex() == FI)
        return true;
    }
  }
  return false;
}

To the best of my knowledge, this code is incorrect. See http://reviews.llvm.org/D15730 with a proposed fix.

and I don't see how a STATEPOINT node (as created in
StatepointLowering.cpp) gets a FixedStackPseudoSourceValue in its
memoperands vector (the loads and stores we generate around the
STATEPOINT node the right aliasing information).

This is done in emitPatchPoint at around 1129 in CodeGen/TargetLoweringBase.cpp

This is done in emitPatchPoint at around 1129 in CodeGen/TargetLoweringBase.cpp

I was only looking at StatepointLowering.cpp -- there we create a
STATEPOINT node without any indication that it may touch the created
spill slots (as opposed to the store and load instructions we create).
However, if the invariant is that "no mem operands == touches the
whole heap", then this is fine.

reames planned changes to this revision.Dec 23 2015, 2:15 PM

This patch needs to be rebased on http://reviews.llvm.org/D15759 "[Statepoints] Use Indirect operands for spill slots" and 256312: "[GC] Make GCStrategy::isGCManagedPointer a type predicate not a value", before further review. Note that the approach described in this review is different from the one I settled on in D15759.

reames updated this revision to Diff 43569.Dec 23 2015, 5:37 PM
reames retitled this revision from [GC][In Progress] Relocating vectors of pointers to [Statepoints] Relocating vectors of pointers.
reames updated this object.
reames removed reviewers: ributzka, qcolombet.

Rebased patch. Ready for review. Removing several reviewers who are likely not interested in the remaining changes since the backend bits were separated and already landed.

pgavlin accepted this revision.Jan 6 2016, 6:35 PM
pgavlin edited edge metadata.
This revision is now accepted and ready to land.Jan 6 2016, 6:35 PM
This revision was automatically updated to reflect the committed changes.