This is an archive of the discontinued LLVM Phabricator instance.

[entry values] ARM: Add a describeLoadedValue override (PR45025)
ClosedPublic

Authored by vsk on Feb 27 2020, 10:22 AM.

Details

Summary

As a narrow stopgap for the assertion failure described in PR45025, add
a describeLoadedValue override to ARMBaseInstrInfo and use it to detect
copies in which the forwarding reg is a super/sub reg of the copy
destination. For the moment this is unsupported.

Several follow ups are possible:

  1. Handle VORRq. At the moment, we do not, because isCopyInstrImpl returns early when !MI.isMoveReg().
  1. In the case where forwarding reg is a super-reg of the copy destination, we should be able to describe the forwarding reg as a subreg within the copy destination. I'm not 100% sure about this, but it looks like that's what's done in AArch64InstrInfo.
  1. In the case where the forwarding reg is a sub-reg of the copy destination, maybe we could describe the forwarding reg using the copy destinaion and a DW_OP_LLVM_fragment (I guess this should be possible after D75036).

https://bugs.llvm.org/show_bug.cgi?id=45025
rdar://59772698

Diff Detail

Event Timeline

vsk created this revision.Feb 27 2020, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2020, 10:22 AM

You may be able to run delta on the MIR testcases.

llvm/lib/Target/ARM/ARMBaseInstrInfo.h
108

doxygen comment?

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
47

we probably don't need most of these?

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir
39

or these?

vsk updated this revision to Diff 247025.Feb 27 2020, 10:31 AM

Delete some unnecessary attributes.

vsk added a comment.Feb 27 2020, 10:35 AM

It looks like the MIR test cases are fairly reduced now, I can give it a shot if there's obvious cruft left?

llvm/lib/Target/ARM/ARMBaseInstrInfo.h
108

I'm not sure what's best to add, here. It looks like the practice has been to copy the doxygen comment from the base class virtual method declaration, but then the documentation could get out of sync.

vsk updated this revision to Diff 247028.Feb 27 2020, 10:41 AM

Add a doxygen comment for the describeLoadedValue override.

aprantl added inline comments.Feb 27 2020, 12:00 PM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
108

Sorry, I missed the override! I'm actually against adding comments to override methods unless they do something unexpected that is not documented in the base class.

aprantl accepted this revision.Feb 27 2020, 12:00 PM
This revision is now accepted and ready to land.Feb 27 2020, 12:00 PM

Oh, now I see what's happening, I think. We're trying to get a description of some double register (like d8). We find the VMOVS because it "defines" d8. But even though VMOVS is a COPY, it's a COPY defining s17, not d8. (Actually, I think there's a missing implicit use, since it's combining s16 and s17 to produce d8.)

Given that, I guess this is the right fix.

This is sort of similar to what the AArch64 code is doing, but not really. There, the high bits are implicitly zeroed, so we don't need to worry about finding some other instruction. Here, we would actually have to merge the definition of s16 with the definition of s17 to produce anything useful.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
1040

Could you simplify this to just if (DstReg != Reg)?

vsk updated this revision to Diff 247093.Feb 27 2020, 1:30 PM

Simplify, per Eli's suggestion.

djtodoro accepted this revision.Feb 27 2020, 11:35 PM

LGTM, thanks! (nits included)

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
40

This is unused, so we can delete it.

71

We can attach only to !23.

80

We do not need all these MF attributes (I think only 'name' and 'callSites').

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir
68

We do not need all these MF attributes (I think only 'name' and 'callSites').

dstenb added inline comments.Feb 28 2020, 4:07 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
1043–1044

This is a problem we're having for our downstream target also, as parameters may be produced by bundled instructions modifying different parts of the register. I have thought a bit about adding an optional Fragment or SubReg field to ParamLoadedValue, to specify which part of the register that was described, and let collectCallSiteParameters() keep the parameter in the worklist until all parts have been described, but I have not really thought more about how that could be implemented.

vsk marked an inline comment as done.Feb 28 2020, 2:29 PM
vsk added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
1043–1044

Interesting. I tried something along these lines to try and have collectCallSiteParameters "merge" the descriptions for different fragments of a forwarding reg. This turned out to be a good bit more complicated than what I had bargained for..

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
71

I'll clean up the test a bit before committing, but we really need a tool to do this kind of reduction. Having narrow/reduced test cases is important, but doing this can cost a fairly outsized amount of engineering time relative to the benefit it delivers.

The number of debug info contributors has grown a lot the past few years, and I hope it keeps growing. For that to happen though we need for an off-the-shelf tool that can help reduce tests. I'll try to prototype something soon.

This revision was automatically updated to reflect the committed changes.
djtodoro added inline comments.Mar 1 2020, 11:30 PM
llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
71

I agree. Such utility could be very useful, and I would like to participate. Thanks!