This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef][1/4] Support transformations that widen or narrow defined values
ClosedPublic

Authored by jmorse on Oct 6 2020, 6:05 AM.

Details

Summary

(That's right, even more patches! This is the third and final patch series for the instruction referencing work, see D85741. Everything "Interesting" should be covered by this and the two prior series, there will be a couple more individual patches to mop up optimisations that drop instruction numbers, and after that we're Done (TM)).

Very late in compilation, backends like X86 will perform optimsations like this:

%cx = MOV16rm $rax, blah blah blah
->
%rcx = MOV64rm $rax, blah blah blah

Widening the load from 16 bits to 64 bits. Seeing how the lower 16 bits remain the same this doesn't affect execution, but is a more compact instruction. However, any debug instruction reference to the defined operand now refers to a 64 bit value, not a 16 bit one, which might be unexpected. Similar things happen earlier in CodeGen:

CALL64pcrel32 @foo, implicit-def $rax
%0:gr64 = COPY $rax
%1:gr32 = COPY %0.sub_32bit

For reasons that will become clear later, we definitely don't want to label any of the COPYs with debug-instr-number. If we care about %1 for debug purposes, then we need to be able to refer to $eax's definition at the CALL64pcrel32, but it defines the wider $rax. This isn't just a matter of width either: some architectures let you refer to the high and low parts of a superregister (such as $al and $ah).

The solution: add a subregister field to the existing "substitutions" facility that lets us point one instruction / operand pair at another. Using the widening example from above, if we had:

%cx = MOV16rm $rax, blah blah blah, debug-instr-number 1 
DBG_INSTR_REF 1, 0

And widened it to be a 64 bit load, record a substitution with a subregister qualifier:

debugValueSubstitutions:
  - { srcinst: 2, srcop: 0, dstinst: 1, dstop: 0, subreg: 4 } 
  
%rcx = MOV64rm $rax, blah blah blah, debug-instr-number 2 
DBG_INSTR_REF 1, 0

The DBG_INSTR_REF still refers to the same number; but a substitution has to be looked up. And that substitution specifies that the sub_16bit field of instruction 1 / operand 0 should be read. These can even be chained.

This patch extends a few APIs to cope with this, and as a demonstration adds the relevant instrumentation to the X86FixupBWInsts pass, which performs the optimisation described above. I've also fiddled with a few tests for substitution stuff now that we've added a new field.

Having established that this feature is needed, the next patch adds support to InstrRefBasedLDV, the third makes use of it when handing COPYs in SelectionDAG, and the fourth does some cleanup.

Diff Detail

Event Timeline

jmorse created this revision.Oct 6 2020, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 6:05 AM
jmorse requested review of this revision.Oct 6 2020, 6:05 AM

curl -L 'https://reviews.llvm.org/D88891?download=1' does not have a/ or b/ prefix. Can you upload a diff with either arc diff, git format-patch -1 or git diff 'HEAD^'? Thanks.

I think arc diff and git format-patch -1 are preferred because they have the base commit information so people can apply this locally with arc patch D88891.

jmorse updated this revision to Diff 300226.Oct 23 2020, 4:33 AM

(Rebase now that I've dropped some "REQUIRES: X86" lines in the modified tests...)

Sorry it took so long to get back to this,

curl -L 'https://reviews.llvm.org/D88891?download=1' does not have a/ or b/ prefix. Can you upload a diff with either arc diff, git format-patch -1 or git diff 'HEAD^'? Thanks.

I think arc diff and git format-patch -1 are preferred because they have the base commit information so people can apply this locally with arc patch D88891.

Hmmm -- I did not know there was that much base / parent information preserved in phab, looking closer I see that other peoples revisions have hashes and parents, wheras mine only have diff numbers. I'll try uploading the output of git-format-patch and see what happens; I've also updated the parent commits to better represent what order these patches need to apply in.

jmorse updated this revision to Diff 300228.Oct 23 2020, 4:43 AM

(rebase again to see if phab will accept more information about base commits and the like)

StephenTozer accepted this revision.Jun 25 2021, 8:53 AM
StephenTozer added a subscriber: StephenTozer.

One small nit/question attached, otherwise LGTM. Given that we don't always correctly follow subregister changes in DBG_VALUEs at the moment, this is a pleasant improvement!

llvm/lib/CodeGen/MIRParser/MIRParser.cpp
429

Complete nit, but is there a reason for the std::make_pair -> {} substitution?

This revision is now accepted and ready to land.Jun 25 2021, 8:53 AM
StephenTozer added inline comments.Jun 25 2021, 9:02 AM
llvm/include/llvm/CodeGen/MachineFunction.h
494

Also not so much a nit or request as a question; should this default value be taken to mean that it is okay for the substitution's subreg to be larger than the source operand, with such a case being treated as a no-op rather than a sext/zext? I'd assume so, since I don't believe that we would ever want to produce a sext/zext using this substitution mechanism - that should only happen as a result of salvaging.

llvm/lib/Target/X86/X86FixupBWInsts.cpp
381

Also one more question about the intended behaviour (that does not block this patch at all), it's potentially expected that we may have to recurse through several substitutions? As is I don't think there's any issue with that, since I believe there is a small lower bound on the number of substitutions we could ever need to make for a single reference, and it seems easier than checking for existing substitutions everywhere that we might create one.

StephenTozer added inline comments.Jun 25 2021, 9:03 AM
llvm/lib/Target/X86/X86FixupBWInsts.cpp
381

s/lower/upper

@jmorse Thanks for doing this. Should we document this new MF attribute somehow? If we already created it, should we add an explanation of this new field there?

From the patch summary, I think that this is reasonable change.

llvm/include/llvm/CodeGen/MachineFunction.h
471

Do we need the std::map sortness here?

llvm/lib/CodeGen/MIRPrinter.cpp
227–234

nit: const auto &Sub ?

jmorse added inline comments.Jun 28 2021, 3:54 AM
llvm/include/llvm/CodeGen/MachineFunction.h
471

Hmmm -- not during the optimisation passes, so this can become a vector that gets sorted when lookups occur.

(I have another patch (not uploaded yet) that might need the sortness, but that can be evaluated in its own right).

494

That would be my interpretation, yeah -- while a reproducer doesn't immediately spring to mind, I can't think of a part of post-isel debug-info that minds if $al is expanded to $rax when it's the operand of a DBG_VALUE. I'm confident I've seen it in the past, but can't put my finger on it right now.

In theory, as future work, we could eliminate that situation if we perfectly instrumented the entire backend with substitution subregister info, but I don't think we would gain anything from that.

llvm/lib/CodeGen/MIRParser/MIRParser.cpp
429

IIRC it's the preferred form, and I was taking the opportunity to fix it up as this line changes anyway.

Not sure why I didn't do the other make_pair, I'll edit that in.

llvm/lib/Target/X86/X86FixupBWInsts.cpp
381

Indeed, multiple substitutions might be made -- the number will be proportionate to the amount of optimisation that has happened to a value, so it shouldn't inflate massively. It's the realisation of the overall theme: don't maintain a perfectly accurate mapping of variable locations during compilation, instead document what happens and piece it together later.

jmorse added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
471

I realised that it's already in-tree with a std::map -- I've written D105029 as a separate patch that un-sorts it.