[PATCH] D37740: [SelectionDAG] Pick correct frame index in LowerArguments

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8791-8794
     // Note down frame index.
     if (FrameIndexSDNode *FI =
         dyn_cast<FrameIndexSDNode>(ArgValues[0].getNode()))
       FuncInfo->setArgumentFrameIndex(&Arg, FI->getIndex());
----------------
rnk wrote:
> Is it possible to fix this condition up so that we don't pattern match the frame index before *and* after the getMergeValues? We should be able to look through LoadSDNodes here to check for a FrameIndexSDNode.
I need to admit that I do not fully understand all the things going on here, and why the setArgumentFrameIndex calls only are done in different special scenarios.
Some examples:
- At line 8759 it is done "for use in FastISel" according to the code comment on line 8756.

- At line 8794 it is done if ArgValues[0] is a FrameIndexSDNode. Is it impossible to end up with ArgValues.size()>1, and also having one of the ArgValues being a FrameIndexSDNode? Or how do we know that it is the FI from ArgValues[0] that should be used in case ArgValues.size()>1?

- And the scenario below (the one that I have tried to improve) is for the very specific case when ArgValues.size()==1 and ArgValue[0] is a BUILD_PAIR (otherwise Res will end up as ISD::MERGE_VALUES). This scenario is also guarded by an explicit check that FastISel isn't enabled.

That last scenario is also limited since it does not dig deep to find the LoadSDNode/FrameIndexSDNode nodes. I actually think that the getCopyFromParts call (which I believe is creating the BUILD_PAIR) could handle more parts than two. So there could be more values involved and not only a single pair.

So what is the effect of not doing setArgumentFrameIndex? Are we just loosing debug info?

And what is the problem with always calling setArgumentFrameIndex? Is it bad for FastISel in some way?

One idea I had was to examine all InVals (for the interval [i .. i+NumParts-1]) just before the ArgValues.push_back() at line 8779.  If any involved part is a FrameIndexSDNode, or a LoasSDNode/FrameIndexSDNode pattern, then I should call setArgumentFrameIndex for the "best" found frame index. If stack grows down then the "best" is the highest index, if stack grows up then the "best" would be the lowest index.
Although, such a solution might seem a little bit hacky. So instead I tried to do a minimal change for the scenario for which I got corrupt debug info in my out-of-tree target.


https://reviews.llvm.org/D37740