This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vkalintiris on Oct 7 2015, 7:24 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vkalintiris updated this revision to Diff 36738.Oct 7 2015, 7:24 AM
vkalintiris retitled this revision from to [mips] Use correct frame register for DWARF info when dynamically realigning the stack..
vkalintiris updated this object.
vkalintiris added a reviewer: dsanders.
vkalintiris added a subscriber: llvm-commits.
dean added a subscriber: dean.Oct 7 2015, 8:01 AM
dean added a comment.Oct 9 2015, 9:21 AM

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?

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?

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 added a comment.Oct 13 2015, 12:43 AM

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 added a comment.Oct 13 2015, 5:06 AM

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

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 edited edge metadata.Oct 16 2015, 3:42 AM

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
588–593 ↗(On Diff #36738)

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

596 ↗(On Diff #36738)

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 marked 2 inline comments as done.
vkalintiris edited edge metadata.

Addressed reviewer comments.

vkalintiris added inline comments.Oct 20 2015, 5:59 AM
lib/Target/Mips/MipsSEFrameLowering.cpp
588–593 ↗(On Diff #36738)

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 updated this revision to Diff 38909.Nov 2 2015, 6:55 AM
  1. Added logic for fixed object indices.
  2. Added test for each separate case.
dsanders accepted this revision.Nov 12 2015, 3:45 AM
dsanders edited edge metadata.

LGTM, thanks

This revision is now accepted and ready to land.Nov 12 2015, 3:45 AM
This revision was automatically updated to reflect the committed changes.