[mips] Use correct frame register for DWARF info when dynamically realigning the stack.

[vkalintiris]

Summary

This patch overrides TargetFrameLowering::getFrameIndexReference() in order to
specify the correct register when the function needs dynamic stack realignment.
The values returned from this function are used in order to create DW_AT_locations
for DWARF info. These locations would use the wrong registers as it's been
reported in PR25028.

[dean]

Hi, thank you for looking into this issue. I've just checked the patch with the tip of llvm but doesn't solve the reported bug. The output is the same as the one reported in the original bug. Is it expected that it requires some additional changes?

[vkalintiris]

In D13511#263702, @dean wrote:

Hi, thank you for looking into this issue. I've just checked the patch with the tip of llvm but doesn't solve the reported bug. The output is the same as the one reported in the original bug. Is it expected that it requires some additional changes?

Is the whole output the same?

[vkalintiris]

I accidentally submitted my reply without finishing it.

Don't you see any difference between the old and the new DWARF info files?

Here's what I get for the test2 subprogram and the char_local variable entries:

<1><98>: Abbrev Number: 3 (DW_TAG_subprogram)
 <99>   DW_AT_low_pc      : 0x80
 <a1>   DW_AT_high_pc     : 0x47c
 <a5>   Unknown AT value: 3fe7: 1
 <a5>   DW_AT_frame_base  : 1 byte block: 6e         (DW_OP_reg30 (r30))
 <a7>   DW_AT_name        : (indirect string, offset: 0xd4): test2
 <ab>   DW_AT_decl_file   : 1
 <ac>   DW_AT_decl_line   : 15
 <ad>   DW_AT_prototyped  : 1
 <ad>   DW_AT_type        : <0x378>
 <b1>   DW_AT_external    : 1

...
...
...

<2><c0>: Abbrev Number: 5 (DW_TAG_variable)
 <c1>   DW_AT_location    : 3 byte block: 8d b8 4    (DW_OP_breg29 (r29): 568)
 <c5>   DW_AT_name        : (indirect string, offset: 0x111): char_local
 <c9>   DW_AT_decl_file   : 1
 <ca>   DW_AT_decl_line   : 17
 <cb>   DW_AT_type        : <0x2a>

[dean]

Hi, sorry I was off yesterday. You've got me, I was assuming you altered the frame base for the function to the $sp. Though I recall I tried with the debugger to print the values and still I've got some of them incorrect. I'll check today what is going wrong.

[dean]

Hi, I actually confirm the patch solves the issue. Apologies for my previous wrong comment, the failure was due to my test. Though a different question, in the dwarf info the matches between the line numbers in the sources and the addresses in the assembly are not much contiguous, with the statements in the source interspersed at different places in the assembly. In general, the assembly looks somewhat optimised. This makes a bit harder to debug the code. Is there something that can be mitigated to obtain a more direct and contiguous correspondence between the original source and the assembly?

Finally, just as curiosity, is there any particular reason you are creating the expression for all the locals as $sp + offset, instead of setting the frame base to the $sp?

Thanks,
Dean

[vkalintiris]

In D13511#265867, @dean wrote:

In general, the assembly looks somewhat optimised. This makes a bit harder to debug the code. Is there something that can be mitigated to obtain a more direct and contiguous correspondence between the original source and the assembly?

I can't think of anything other than trying to use lower optimization levels.

Finally, just as curiosity, is there any particular reason you are creating the expression for all the locals as $sp + offset, instead of setting the frame base to the $sp?

When we use dynamic stack realignment we have to use $sp for only locals and $fp for incoming params, ie. conceptually the $fp is really the frame base register. This matches the behaviour of the other targets that I checked too.

[dsanders]

In general, the assembly looks somewhat optimised. This makes a bit harder to debug the code. Is there something that can be mitigated to obtain a more direct and contiguous correspondence between the original source and the assembly?

I can't think of anything other than trying to use lower optimization levels.

ouch, I'm already invoking llc with -O0. Does this depend on the front-end?

I'm aware that SelectionDAG will fold identical nodes together, effectively doing CSE.

You could try '-mllvm -pre-RA-sched=source' which tries to respect source order when linearizing the SelectionDAG nodes.

lib/Target/Mips/MipsSEFrameLowering.cpp

596–601

I'm surprised that the frame pointer isn't a possible choice here. Shouldn't MipsRegisterInfo::getFrameRegister() be called somewhere?

604

Indentation? I'm not sure on this one but I'd expect clang-format to align to the MFI.

test/CodeGen/Mips/dynamic-stack-realignment-dwarf.ll

1 ↗

(On Diff #36738)

(filename): This test should be in test/DebugInfo/Mips

[vkalintiris]

Addressed reviewer comments.

[vkalintiris]

lib/Target/Mips/MipsSEFrameLowering.cpp

596–601

MipsRegisterInfo::getFrameRegister() computes the frame register ($sp or $fp). Here, we want to calculate the register + offset used for a specific frame index location. This can also include the base register $s7 when using dynamic stack realignment + VLAs.

[vkalintiris]

1. Added logic for fixed object indices.
2. Added test for each separate case.

[dsanders]

LGTM, thanks

lib/Target/Mips/MipsSEFrameLowering.h

 public:
   explicit MipsSEFrameLowering(const MipsSubtarget &STI);
 
   /// emitProlog/emitEpilog - These methods insert prolog and epilog code into
   /// the function.
   void emitPrologue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
   void emitEpilogue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
 
+  int getFrameIndexReference(const MachineFunction &MF, int FI,
+                             unsigned &FrameReg) const override;
+
   bool spillCalleeSavedRegisters(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MI,
                                  const std::vector<CalleeSavedInfo> &CSI,
                                  const TargetRegisterInfo *TRI) const override;
 
   bool hasReservedCallFrame(const MachineFunction &MF) const override;
 
   void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs,
                             RegScavenger *RS) const override;
   unsigned ehDataReg(unsigned I) const;
 };
 
 } // End llvm namespace
 
 #endif

lib/Target/Mips/MipsSEFrameLowering.cpp

void MipsSEFrameLowering::emitEpilogue(MachineFunction &MF,
 
   if (!StackSize)
     return;
 
   // Adjust stack.
   TII.adjustStackPtr(SP, StackSize, MBB, MBBI);
 }
 
+int MipsSEFrameLowering::getFrameIndexReference(const MachineFunction &MF,
+                                                int FI,
+                                                unsigned &FrameReg) const {
+  const MachineFrameInfo *MFI = MF.getFrameInfo();
+  const MipsRegisterInfo *MRI = STI.getRegisterInfo();
+  MipsABIInfo ABI = STI.getABI();
+
+  // Find out the correct register.
+  FrameReg = ABI.GetStackPtr();
+  if (hasBP(MF))
+    FrameReg = ABI.GetBasePtr();
+  else if (MRI->needsStackRealignment(MF))
+    FrameReg = ABI.GetStackPtr();
+
+  return MFI->getObjectOffset(FI) + MFI->getStackSize() -
+         getOffsetOfLocalArea() + MFI->getOffsetAdjustment();
+}
+
 bool MipsSEFrameLowering::
 spillCalleeSavedRegisters(MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator MI,
                           const std::vector<CalleeSavedInfo> &CSI,
                           const TargetRegisterInfo *TRI) const {
   MachineFunction *MF = MBB.getParent();
   MachineBasicBlock *EntryBlock = &MF->front();
   const TargetInstrInfo &TII = *STI.getInstrInfo();

test/DebugInfo/Mips/dynamic-stack-realignment-dwarf.ll

+; RUN: llc -filetype=obj -march=mips -mcpu=mips32r2 < %s | \
+; RUN:    llvm-dwarfdump -debug-dump=info - | FileCheck %s
+;
+; Verify that we use the correct DW_OP_bregXX when dynamic stack realignment
+; is in use (PR25028).
+;
+;#include <alloca.h>
+;
+;int foo(int a, int b) {
+;  int c __attribute__ ((aligned (16))) = a + b;
+;  return c;
+;}
+;
+;int *bar(int x, int y) {
+;  int *w = alloca(x);
+;  int z __attribute__ ((aligned (16))) = x + y;
+;  *w = z;
+;  return w;
+;}
+;
+
+; DW_OP_reg30
+; CHECK:      DW_AT_frame_base [DW_FORM_exprloc]      (<0x1> 6e )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "foo")
+
+; DW_OP_breg29
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 8d 08 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "a"
+
+; DW_OP_breg29
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 8d 04 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "b"
+
+; DW_OP_breg29
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 8d 00 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "c"
+
+define i32 @foo(i32 signext %a, i32 signext %b) #0 {
+entry:
+  %a.addr = alloca i32, align 4
+  %b.addr = alloca i32, align 4
+  %c = alloca i32, align 16
+  store i32 %a, i32* %a.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %a.addr, metadata !15, metadata !16), !dbg !17
+  store i32 %b, i32* %b.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %b.addr, metadata !18, metadata !16), !dbg !19
+  call void @llvm.dbg.declare(metadata i32* %c, metadata !20, metadata !16), !dbg !21
+  %0 = load i32, i32* %a.addr, align 4, !dbg !22
+  %1 = load i32, i32* %b.addr, align 4, !dbg !23
+  %add = add nsw i32 %0, %1, !dbg !24
+  store i32 %add, i32* %c, align 16, !dbg !21
+  %2 = load i32, i32* %c, align 16, !dbg !25
+  ret i32 %2, !dbg !26
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; DW_OP_reg30
+; CHECK:      DW_AT_frame_base [DW_FORM_exprloc]      (<0x1> 6e )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "bar")
+
+; DW_OP_breg23
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 87 14 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "x"
+
+; DW_OP_breg23
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 87 10 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "y"
+
+; DW_OP_breg23
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 87 0c )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "w"
+
+; DW_OP_breg23
+; CHECK:      DW_AT_location [DW_FORM_exprloc]       (<0x2> 87 00 )
+; CHECK-NEXT: DW_AT_name [DW_FORM_strp]{{.*}} "z"
+
+define i32* @bar(i32 signext %x, i32 signext %y) #0 {
+entry:
+  %x.addr = alloca i32, align 4
+  %y.addr = alloca i32, align 4
+  %w = alloca i32*, align 4
+  %z = alloca i32, align 16
+  store i32 %x, i32* %x.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !27, metadata !16), !dbg !28
+  store i32 %y, i32* %y.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %y.addr, metadata !29, metadata !16), !dbg !30
+  call void @llvm.dbg.declare(metadata i32** %w, metadata !31, metadata !16), !dbg !32
+  %0 = load i32, i32* %x.addr, align 4, !dbg !33
+  %1 = alloca i8, i32 %0, !dbg !33
+  %2 = bitcast i8* %1 to i32*, !dbg !33
+  store i32* %2, i32** %w, align 4, !dbg !32
+  call void @llvm.dbg.declare(metadata i32* %z, metadata !34, metadata !16), !dbg !35
+  %3 = load i32, i32* %x.addr, align 4, !dbg !36
+  %4 = load i32, i32* %y.addr, align 4, !dbg !37
+  %add = add nsw i32 %3, %4, !dbg !38
+  store i32 %add, i32* %z, align 16, !dbg !35
+  %5 = load i32, i32* %z, align 16, !dbg !39
+  %6 = load i32*, i32** %w, align 4, !dbg !40
+  store i32 %5, i32* %6, align 4, !dbg !41
+  %7 = load i32*, i32** %w, align 4, !dbg !42
+  ret i32* %7, !dbg !43
+}
+
+attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="mips32r2" "target-features"="+fpxx,+mips32r2,+nooddspreg" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!12, !13}
+!llvm.ident = !{!14}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.8.0 (trunk 249525) (llvm/trunk 249528)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "hello.c", directory: "/home/vk/repos/tmp/hello")
+!2 = !{}
+!3 = !{!4, !8}
+!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !5, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, function: i32 (i32, i32)* @foo, variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{!7, !7, !7}
+!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!8 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 8, type: !9, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, function: i32* (i32, i32)* @bar, variables: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !7, !7}
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 32, align: 32)
+!12 = !{i32 2, !"Dwarf Version", i32 4}
+!13 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{!"clang version 3.8.0 (trunk 249525) (llvm/trunk 249528)"}
+!15 = !DILocalVariable(name: "a", arg: 1, scope: !4, file: !1, line: 3, type: !7)
+!16 = !DIExpression()
+!17 = !DILocation(line: 3, column: 13, scope: !4)
+!18 = !DILocalVariable(name: "b", arg: 2, scope: !4, file: !1, line: 3, type: !7)
+!19 = !DILocation(line: 3, column: 20, scope: !4)
+!20 = !DILocalVariable(name: "c", scope: !4, file: !1, line: 4, type: !7)
+!21 = !DILocation(line: 4, column: 7, scope: !4)
+!22 = !DILocation(line: 4, column: 42, scope: !4)
+!23 = !DILocation(line: 4, column: 46, scope: !4)
+!24 = !DILocation(line: 4, column: 44, scope: !4)
+!25 = !DILocation(line: 5, column: 10, scope: !4)
+!26 = !DILocation(line: 5, column: 3, scope: !4)
+!27 = !DILocalVariable(name: "x", arg: 1, scope: !8, file: !1, line: 8, type: !7)
+!28 = !DILocation(line: 8, column: 14, scope: !8)
+!29 = !DILocalVariable(name: "y", arg: 2, scope: !8, file: !1, line: 8, type: !7)
+!30 = !DILocation(line: 8, column: 21, scope: !8)
+!31 = !DILocalVariable(name: "w", scope: !8, file: !1, line: 9, type: !11)
+!32 = !DILocation(line: 9, column: 8, scope: !8)
+!33 = !DILocation(line: 9, column: 12, scope: !8)
+!34 = !DILocalVariable(name: "z", scope: !8, file: !1, line: 10, type: !7)
+!35 = !DILocation(line: 10, column: 7, scope: !8)
+!36 = !DILocation(line: 10, column: 42, scope: !8)
+!37 = !DILocation(line: 10, column: 46, scope: !8)
+!38 = !DILocation(line: 10, column: 44, scope: !8)
+!39 = !DILocation(line: 11, column: 8, scope: !8)
+!40 = !DILocation(line: 11, column: 4, scope: !8)
+!41 = !DILocation(line: 11, column: 6, scope: !8)
+!42 = !DILocation(line: 12, column: 10, scope: !8)
+!43 = !DILocation(line: 12, column: 3, scope: !8)