[PATCH] D15632: [GC][In Progress] Relocating vectors of pointers

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 22 16:46:17 PST 2015


reames added a comment.

In http://reviews.llvm.org/D15632#313619, @sanjoy wrote:

> 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


http://reviews.llvm.org/D15632





More information about the llvm-commits mailing list