This is an archive of the discontinued LLVM Phabricator instance.

[CSInfo][MIPS] Call delay slot support in DwarfDebug
ClosedPublic

Authored by ntesic on Apr 14 2020, 6:41 AM.

Details

Summary

Added call site info support for call instructions with delay slot.
Search for instructions inside call delay slot, which load value
into parameter forwarding registers.
Return address of the call points to instruction after call delay slot,
which is not the one, immediately after the call instruction.

Tests:
-dbg-call-site-delay-slot-interpretation.mir, dbg-call-site-delay-slot-interpretation-64.mir - confirm creation of call_site_parameter DIE for parameters loaded in call delay slot
-dbg-call-site-low-pc.ll - confirm call return address (DW_AT_low_pc) of call_site DIE (label after call delay slot)

Diff Detail

Event Timeline

ntesic created this revision.Apr 14 2020, 6:41 AM
dstenb added inline comments.Apr 14 2020, 10:09 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
799

Nit: s/forwaring/forwarding

805–806

Might there be some target where a parameter value is produced by a series of delay slot instructions? If so, should this search start from the bottom of the delay slot bundle?

djtodoro added inline comments.Apr 23 2020, 2:43 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
805–806

I guess that we are operating with the targets with only one instruction within delay slot. I think we can get rid of the for loop, and add a comment + assert() describing that we expect only one instruction within the bundle.

@dstenb Does that sound OK to you?

dstenb added inline comments.Apr 27 2020, 7:22 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
805–806

That sounds good to me!

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 7:22 AM
ntesic updated this revision to Diff 260578.Apr 28 2020, 3:19 AM

Address comments.
@dstenb @djtodoro Thanks for your suggestions.

ntesic marked 2 inline comments as done.Apr 28 2020, 3:20 AM
dstenb added inline comments.Apr 30 2020, 7:08 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
853–854

Do you know what would happen if you didn't have this guard, and enabled call site information for another target with delay slots?

If it is something that is relatively easy to catch, or perhaps not very serious, I think I'd prefer to have this code target-agnostic (i.e. just MI.hasDelaySlot), and just leave it up to the developer enabling call site information for the target to make sure that there are no delay slot quirks that need to be considered.

1867–1868

Same as above.

vsk added inline comments.May 7 2020, 11:19 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
853–854

+ 1

djtodoro added inline comments.May 8 2020, 6:40 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
853–854

+1, but I think we should at least make an assertion describing what we currently expect, e.g. bundled call instruction should be mips-call-bundle like:

call {
   delay slot
  }
label
llvm/test/DebugInfo/MIR/Mips/dbg-call-site-delay-slot-interpretation-64bit.mir
94

Please use only one DILocation per scope.
e.g. attach every {!21, !22, !23, !24} dbg! to !21

The main purpose of the "delay slot guard" is to ensure that call_site DIE won't be created for call with delay slot, because it could have incorrect "return PC" value (DW_AT_low_pc, DW_AT_call_return_pc).
This will happen if the call and its delay slot instruction are not in the expected order.

Result of the wrong instruction order in call-delay slot bundle is incorrect label insertion in DwarfDebug::beginInstruction().
Label will be inserted immediately after call instruction, but it should be inserted after delay slot instruction.
Please, notice label "$tmp2" after call instruction "jal" in the following assembler output.

jal	f1
$tmp2:
	.loc	1 6 11                  # m.c:6:11
	sw	$4, %lo(global)($1)

During creation of call_site DIEs in DwarfDebug::constructCallSiteEntryDIEs(), the incorrectly placed label is used and wrong value of "returnPC" interpreted.
So, the call return address points to the delay slot instruction and it should point to the instruction after.
Please, find value of "DW_AT_low_pc" which points to delay slot instruction "sw $4 ..." in the following dwarfdump and objdump outputs.

0x00000067:     DW_TAG_GNU_call_site
                  DW_AT_abstract_origin	(0x00000079 "f1")
                  DW_AT_low_pc	(0x0000000000000010)
c: 0c 00 00 00                  	jal	0 <foo>
      10: ac 24 00 00                  	sw	$4, 0($1)
      14: 24 02 00 7b                  	addiu	$2, $zero, 123 <foo+0x7b>

This problem for MIPS architecture was caused in LiveDebugValues pass, by insertion of DBG_VALUE after the call instruction, instead of insertion after whole bundle.
Please, find the following output after livedebugvalues pass, clearly incorrect.

JAL @f1 ...{
}
DBG_VALUE $a0
SW renamable $a0

Fortunately, the livedebugvalues problem was fixed by https://reviews.llvm.org/D66888.

Do you know what would happen if you didn't have this guard, and enabled call site information for another target with delay slots?

So, if the "call-delay slot" instruction order is not as expected, it will cause wrong "return PC" value.

If it is something that is relatively easy to catch, or perhaps not very serious, I think I'd prefer to have this code target-agnostic (i.e. just MI.hasDelaySlot), and just leave it up to the developer enabling call site information for the target to make sure that there are no delay slot quirks that need to be considered.

We can use assertions in "delaySlotSupported" lambda, in order to ensure expected order of instructions (following example), instead of the target check.

call_instruction {
   delay_slot_instruction
  }

In beginInstruction() guard we should expect that delay slot instruction is immediately after call instruction, before label insertion.
In constructCallSiteEntryDIEs() guard we should expect that both the call instruction and its successor are followed by the same label.

@dstenb @vsk @djtodoro Thanks for your comments and, please,
let me know what do you think about this approach.

In beginInstruction() guard we should expect that delay slot instruction is immediately after call instruction, before label insertion.
In constructCallSiteEntryDIEs() guard we should expect that both the call instruction and its successor are followed by the same label.

@dstenb @vsk @djtodoro Thanks for your comments and, please,
let me know what do you think about this approach.

Sorry for the delay! Adding such assertions sounds good to me.

ntesic updated this revision to Diff 265496.May 21 2020, 6:59 AM

Address comments.
Change of delay slot guard - use assertions instead of target check.

ntesic marked 5 inline comments as done.May 21 2020, 7:02 AM
ntesic updated this revision to Diff 265499.May 21 2020, 7:07 AM

Capture "&" instead of "this" in delaySlotSupported lambda.

Minor comments. Looks good to me otherwise!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
851

I think it might be worth making this an early exit:

if (!MI.isBundledWithSucc())
  return false;
878

I think this might read a bit as that we expect a delay slot instruction here. Could we perhaps break out the hasDelaySlot() check to here:

if (MI.hasDelaySlot() && !delaySlotSupported(*&MI))
  return;

or perhaps instead rename the delaySlotSupported() function to indicate this (I don't have any better suggestion).

1865

Same as above.

1875

Same as above.

ntesic updated this revision to Diff 266025.May 25 2020, 8:21 AM

Address comment - delaySlotSupported change.
Compress test-CHECK-lines.

ntesic marked 5 inline comments as done.May 25 2020, 8:24 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
878

I agree, it seems more intuitive. Thanks for the suggestion.

ntesic marked an inline comment as done and 3 inline comments as not done.May 25 2020, 8:26 AM

@dstenb Thanks for the review. Any additional comments?

dstenb accepted this revision.May 29 2020, 4:21 AM

LGTM. Sorry for the delay!

This revision is now accepted and ready to land.May 29 2020, 4:21 AM
This revision was automatically updated to reflect the committed changes.