[PATCH] D18106: Extract out a SelectionDAGBuilder::LowerAsStatepoint; NFC

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:511
@@ -591,3 +510,3 @@
   // the alloca
-  for (Value *V : StatepointSite.gc_args()) {
+  for (Value *V : SI.GCArgs) {
     SDValue Incoming = Builder.getValue(V);
----------------
sanjoy wrote:
> reames wrote:
> > This comment still applies.  A follow on commit is fine.
> Ah, sorry, I somehow missed seeing the comment earlier.  I'll check in a follow-on change.
Thinking about it, the current behavior seems somewhat problematic --
whether an incoming gc pointer gets the special "already a spill slot"
treatment depends on whether SelectionDAG lowers it to a
`FrameIndexSDNode` or not; and that in turn depends on whether the
`llvm::Value` is statically an `llvm::AllocaInst` or not.  So if I
start with a `gc.statepoint` that has an `alloca` as a gc pointer,
something like LCSSA (say) could obscure the `llvm::AllocaInst` behind
a PHI, and `getValue` will give us a `CopyFromReg`, and this would
change behavior.

I think even in the best case we can't do what this bit of code is
trying to do (assuming I understand its intent correctly) -- you could
have a PHI of two //different// allocas flowing in, into the
`gc.statepoint` (this could have been created due to tail merging, for
instance) and then we won't even have a fixed frame index that we can
report.

The caveat to the above is that I've assumed that the special
treatment of alloca's is needed for correctness.  If this is merely a
performance optimization, then the problem is easier.


Repository:
  rL LLVM

http://reviews.llvm.org/D18106