[PATCH] D53889: [CodeGen] Prefer static frame index for STATEPOINT liveness args

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In addition to the detailed implementation comment below, I thought of a possible semantic problem.  Per the ABI, who "owns" the memory for the arguments?  Does either the callee or caller assume it's immutable?  If so, then your optimization needs to be restricted to a non-relocating collector since otherwise the collector might be updating a memory location assumed by AA to be immutable and bad things could happen...

I'm 97% sure we do assume arguments are immutable within a function...



================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:529
+        (isa<Argument>(V) &&
+         Builder.FuncInfo.getArgumentFrameIndex(cast<Argument>(V)) != INT_MAX))
+      Incoming = Builder.getNonRegisterValue(V);
----------------
cherry wrote:
> reames wrote:
> > Hm, I'm really uncomfortable with this bit of code.  For two reasons:
> > 1) I agree with the general idea this should be generic and buried inside getValue.  If it can't be, it makes me want to understand why because I'm missing something.
> > 2) You shouldn't need to change handling of allocas since that should already work.
> > 
> > Is it possible to let the virtual register be lowered and then peak back through to the source?  
> > 
> > Basically, I'm looking to be convinced this is the right approach.
> You're right that allocas already work. I'll remove that case.
> 
> I'm happy to try other approach, either trying harder in doing this in getValue, or some other way.
> 
I have two alternatives I'd like you to try.  Both start with removing the code from the generic getNonRegisterValue since I'm unsure about the implications of that.

Option 1 - Right here, just check to see if the Value V is an argument of the right type and inline the logic from getNonRegisterValue into the only user.  

Option 2 - Inside lowerIncomingStatepointValue, add a special case which matches the DAG pattern Load(FrameIndex) and uses the optimized lowering.  There are examples of things like this in the argument handling in SelectionDAG, the rough structure will look something like:
  if (N.getNode())
    // Check if frame index is available.
    if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(N.getNode()))
      if (FrameIndexSDNode *FINode =
          dyn_cast<FrameIndexSDNode>(LNode->getBasePtr().getNode()))
        Op = MachineOperand::CreateFI(FINode->getIndex());


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

https://reviews.llvm.org/D53889