This is an archive of the discontinued LLVM Phabricator instance.

[MachineCombiner] Preserve debug instruction number
ClosedPublic

Authored by fdeazeve on Mar 9 2023, 7:36 PM.

Details

Summary

Each target's TargetInstrInfo is responsible for announcing which code
patterns it is able to transform during the MachineCombiner pass.
Currently, these patterns are applied without preserving the debug
instruction number required by the InstrRef implementation of
LiveDebugValues. As such, we've seen a number of examples where debug
information is dropped for variables in InstrRef mode that were
otherwise available in VarLoc mode. This has been observed both in X86
and AArch examples.

This commit is an initial attempt at preserving said numbers by changing
the general (target agnostic) implementation of TargetInstrInfo: the
reassociation pattern must keep the debug number of the "top level"
instruction, i.e., the instruction whose value represents the final
value of the arithmetic expression. Intermediate values must have their
debug number dropped, as they have no equivalent value in the
unoptimized code.

Future work is required to update each target's
TargetInstrInfo::genAlternativeCodeSequence method.

Diff Detail

Event Timeline

fdeazeve created this revision.Mar 9 2023, 7:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 7:36 PM
fdeazeve requested review of this revision.Mar 9 2023, 7:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 7:36 PM
fdeazeve updated this revision to Diff 504010.Mar 9 2023, 7:37 PM

Simplify test

fdeazeve updated this revision to Diff 504012.Mar 9 2023, 7:39 PM

Remove WIP comments.

fdeazeve added inline comments.Mar 9 2023, 7:45 PM
llvm/test/CodeGen/X86/machine-combiner-dbg.ll
1

FWIW this is heavily based on llvm/test/CodeGen/X86/machine-combiner.ll

fdeazeve updated this revision to Diff 504014.Mar 9 2023, 7:57 PM

Fix typo in commit message

fdeazeve edited the summary of this revision. (Show Details)Mar 9 2023, 7:58 PM
jmorse accepted this revision.Mar 10 2023, 2:11 AM

LGTM; you might consider making it a MIR test that just runs the machine-combiner pass, but I don't think that'll have any material advantage over an .ll test, the only difference is making the exact input to machine-combiner explicitly visible.

This revision is now accepted and ready to land.Mar 10 2023, 2:11 AM
fdeazeve updated this revision to Diff 504118.Mar 10 2023, 6:02 AM

Convert test into a MIR test that _only_ runs machine combiner

LGTM; you might consider making it a MIR test that just runs the machine-combiner pass

Agreed! This makes the test less susceptible to changes elsewhere in the backend.

This revision was automatically updated to reflect the committed changes.