[SelectionDAG] Pick correct frame index in LowerArguments

[bjope]

Summary

SelectionDAGISel::LowerArguments is associating arguments
with frame indices (FuncInfo->setArgumentFrameIndex). That
information is later on used by EmitFuncArgumentDbgValue to
create DBG_VALUE instructions that denotes that a variable
can be found on the stack.

I discovered that for our (big endian) out-of-tree target
the association created by SelectionDAGISel::LowerArguments
sometimes is wrong. I've seen this happen when a 64-bit value
is passed on the stack. The argument will occupy two stack
slots (frame index X, and frame index X+1). The fault is
that a call to setArgumentFrameIndex is associating the
64-bit argument with frame index X+1. The effect is that the
debug information (DBG_VALUE) will point at the least significant
part of the arguement on the stack. When printing the
argument in a debugger I will get the wrong value.

I managed to create a test case for PowerPC that seems to
show the same kind of problem.

The bugfix will look at the datalayout, taking endianess into
account when examining a BUILD_PAIR node, assuming that the
least significant part is in the first operand of the BUILD_PAIR.
For big endian targets we should use the frame index from
the second operand, as the most significant part will be stored
at the lower address (using the highest frame index).

I also added an assert that we always pick the operand associated
with the highest frame index in this particular piece of code.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8802 ↗

(On Diff #114819)

Always the highest or does this depend on the direction in which the stack grows?

8811 ↗

(On Diff #114819)

can you factor the ternary operator out into the definition of BigEndian?

test/CodeGen/PowerPC/debuginfo-stackarg.ll

1 ↗

(On Diff #114819)

can this be further localized by adding stop-after=live-debug-values?

45 ↗

(On Diff #114819)

please strip all non-essential attributes (everything in quotes?)

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8802 ↗

(On Diff #114819)

Oh wait. I guess the frame indices are numbered "in the same way" even if the stack is growing in the other direction (considering negative/positive frame indices etc). So we probably want to find the lowest FI if the stack grows up.
So the comparison in the assert needs to be based on stack direction. And my comment is confusing at the moment. But I hope the pair will be built in the same way (regarding which part that contains the least/most significant bytes), so the rest should be ok.

[rnk]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8791–8794 ↗

(On Diff #114819)

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.

test/CodeGen/PowerPC/debuginfo-stackarg.ll

45 ↗

(On Diff #114819)

Eventually I'd like to make a pass that "canonicalizes" LLVM IR from clang to strip out all this hyper-target-specific junk (PIC size, wchar_size, regparms, etc). It would also remove the optnone attribute which continues to trip me up when I'm trying to generate "simple" LLVM IR for tests with clang -O0. I have to do clang -O1 -Xclang -disable-llvm-passes, and then I remove the lifetime markers because they are mostly noise for debug info.

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8791–8794 ↗

(On Diff #114819)

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.

[bjope]

Some fixes related to comments from Adrian Prantl:

• Strip some attributes from the test case.
• Stop test after livedebugvalues (that made it possible to relate checks to positions in the fixedStack instead of load instructions).
• Reformulated the code comment that did not take stack growth direction into consideration.
• Removed the assert comparing frame index with for the "other" BUILD_PAIR operand. I think we can live without it, as it got a little bit more complicated when taking stack growth into account.
• Factored out the "LowAddressOp" number to enhance readability.

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8791–8794 ↗

(On Diff #114819)

A shorter version of my earlier answer:

• Yes, the code can probably be refactored.
• It would be nice to refactor it in some way, because it seems incomplete and hard to understand.

Although, I'm not sure that I feel comfortable with doing such refactoring right now. I've just recently started to dig into how debug-info works in LLVM. Give me a few weeks (or months) and maybe I will be ready for the task. But I would definitely not mind if someone with more experience in this area could dig in to it.

[bjope]

I'd still like to contribute by landing this bugfix. We are using it for our out-of-tree target and the problem seems to exist for other big-endian targets as well, as shown by the test case for powerpc.
Any objections?
Ping!

[aprantl]

Seems ok to me.
@rnk: was your concern addressed?

[rnk]

In D37740#877735, @aprantl wrote:

Seems ok to me.
@rnk: was your concern addressed?

Yeah, sorry I didn't say anything. My thoughts were along the lines of "yes, this code is complicated and hard to simplify. Bummer. :("

[bjope]

In D37740#877747, @rnk wrote:

In D37740#877735, @aprantl wrote:

Seems ok to me.
@rnk: was your concern addressed?

Yeah, sorry I didn't say anything. My thoughts were along the lines of "yes, this code is complicated and hard to simplify. Bummer. :("

Thanks for the review! Hopefully we can come back and simplify this code in the future.

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (FrameIndexSDNode *FI =
         dyn_cast<FrameIndexSDNode>(ArgValues[0].getNode()))
       FuncInfo->setArgumentFrameIndex(&Arg, FI->getIndex());
 
     SDValue Res = DAG.getMergeValues(makeArrayRef(ArgValues.data(), NumValues),
                                      SDB->getCurSDLoc());
 
     SDB->setValue(&Arg, Res);
     if (!TM.Options.EnableFastISel && Res.getOpcode() == ISD::BUILD_PAIR) {
+      // We want to associate the argument with the frame index, among
+      // involved operands, that correspond to the lowest address. The
+      // getCopyFromParts function, called earlier, is swapping the order of
+      // the operands to BUILD_PAIR depending on endianness. The result of
+      // that swapping is that the least significant bits of the argument will
+      // be in the first operand of the BUILD_PAIR node, and the most
+      // significant bits will be in the second operand.
+      unsigned LowAddressOp = DAG.getDataLayout().isBigEndian() ? 1 : 0;
       if (LoadSDNode *LNode =
-          dyn_cast<LoadSDNode>(Res.getOperand(0).getNode()))
+          dyn_cast<LoadSDNode>(Res.getOperand(LowAddressOp).getNode()))
         if (FrameIndexSDNode *FI =
             dyn_cast<FrameIndexSDNode>(LNode->getBasePtr().getNode()))
           FuncInfo->setArgumentFrameIndex(&Arg, FI->getIndex());
     }
 
     // Update the SwiftErrorVRegDefMap.
     if (Res.getOpcode() == ISD::CopyFromReg && isSwiftErrorArg) {
       unsigned Reg = cast<RegisterSDNode>(Res.getOperand(1))->getReg();
       if (TargetRegisterInfo::isVirtualRegister(Reg))
         FuncInfo->setCurrentSwiftErrorVReg(FuncInfo->MBB,
                                            FuncInfo->SwiftErrorArg, Reg);

llvm/trunk/test/CodeGen/PowerPC/debuginfo-stackarg.ll

+; RUN: llc < %s -O1 -stop-after=livedebugvalues -o - | FileCheck %s
+
+; This ll-file was created by:
+;   clang --target=powerpc-apple-darwin9 -O1 -S -g -emit-llvm debuginfo-stackarg.c
+;
+; with debuginfo-stackarg.c being the program:
+;   long long foo(long long bar1, long long bar2, long long bar3, long long bar4, long long bar5)
+;   {
+;     return bar1 + bar2 + bar3 + bar4 + bar5;
+;   }
+
+; ModuleID = 'debuginfo-stackarg.c'
+source_filename = "debuginfo-stackarg.c"
+target datalayout = "E-m:o-p:32:32-f64:32:64-n32"
+target triple = "powerpc-apple-macosx10.5.0"
+
+; Function Attrs: nounwind readnone ssp uwtable
+define i64 @foo(i64 %bar1, i64 %bar2, i64 %bar3, i64 %bar4, i64 %bar5) local_unnamed_addr #0 !dbg !8 {
+; Variable bar5 should be associated with a position on the stack (offset relative r1).
+; Let's verify that we point out the start address (lowest address).
+;
+; First find the metadata id for bar5.
+; CHECK: !17 = !DILocalVariable(name: "bar5", arg: 5
+;
+; Now check that we got two entries on the fixed stack with "expected" offsets.
+; CHECK-LABEL: fixedStack:
+; CHECK: id: 0, type: default, offset: 60, size: 4
+; CHECK: id: 1, type: default, offset: 56, size: 4
+; CHECK-NOT: id: 2
+; CHECK-LABEL: stack:
+;
+; Finally check the resulting function body.
+; We expect to find a DBG_VALUE refering to the metadata id for bar5, using the lowest
+; of the two fixed stack offsets found earlier.
+; CHECK-LABEL: body:
+; CHECK: DBG_VALUE %r1, 0, !17, !DIExpression(DW_OP_plus_uconst, 56)
+entry:
+  tail call void @llvm.dbg.value(metadata i64 %bar1, metadata !13, metadata !DIExpression()), !dbg !18
+  tail call void @llvm.dbg.value(metadata i64 %bar2, metadata !14, metadata !DIExpression()), !dbg !19
+  tail call void @llvm.dbg.value(metadata i64 %bar3, metadata !15, metadata !DIExpression()), !dbg !20
+  tail call void @llvm.dbg.value(metadata i64 %bar4, metadata !16, metadata !DIExpression()), !dbg !21
+  tail call void @llvm.dbg.value(metadata i64 %bar5, metadata !17, metadata !DIExpression()), !dbg !22
+  %add = add nsw i64 %bar2, %bar1, !dbg !23
+  %add1 = add nsw i64 %add, %bar3, !dbg !24
+  %add2 = add nsw i64 %add1, %bar4, !dbg !25
+  %add3 = add nsw i64 %add2, %bar5, !dbg !26
+  ret i64 %add3, !dbg !27
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind readnone ssp uwtable }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 6.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "debuginfo-stackarg.c", directory: "/repo")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 2}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 6.0.0"}
+!8 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !9, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11, !11, !11, !11, !11}
+!11 = !DIBasicType(name: "long long int", size: 64, encoding: DW_ATE_signed)
+!12 = !{!13, !14, !15, !16, !17}
+!13 = !DILocalVariable(name: "bar1", arg: 1, scope: !8, file: !1, line: 1, type: !11)
+!14 = !DILocalVariable(name: "bar2", arg: 2, scope: !8, file: !1, line: 1, type: !11)
+!15 = !DILocalVariable(name: "bar3", arg: 3, scope: !8, file: !1, line: 1, type: !11)
+!16 = !DILocalVariable(name: "bar4", arg: 4, scope: !8, file: !1, line: 1, type: !11)
+!17 = !DILocalVariable(name: "bar5", arg: 5, scope: !8, file: !1, line: 1, type: !11)
+!18 = !DILocation(line: 1, column: 25, scope: !8)
+!19 = !DILocation(line: 1, column: 41, scope: !8)
+!20 = !DILocation(line: 1, column: 57, scope: !8)
+!21 = !DILocation(line: 1, column: 73, scope: !8)
+!22 = !DILocation(line: 1, column: 89, scope: !8)
+!23 = !DILocation(line: 3, column: 15, scope: !8)
+!24 = !DILocation(line: 3, column: 22, scope: !8)
+!25 = !DILocation(line: 3, column: 29, scope: !8)
+!26 = !DILocation(line: 3, column: 36, scope: !8)
+!27 = !DILocation(line: 3, column: 3, scope: !8)