Page MenuHomePhabricator

[DebugInfo][InstrRef] Turn instruction referencing on by default for x86
ClosedPublic

Authored by jmorse on Nov 26 2021, 4:21 AM.

Details

Summary

This patch makes instruction referencing on-by-default for x86, as discussed in https://lists.llvm.org/pipermail/llvm-dev/2021-November/153653.html . I'm sure there'll be an amount of unexpected breakage, so I'll land in two parts, with a minimal sized patch switching the default so that reverting is easy.

It seems there are a number of complicated interactions between the cc1 arguments and the codegen flags -- I don't really want to untangle them, so I've just removed the cc1 argument for instruction referencing. It'll still be accessible with -mllvm -experimental-debug-variable-locations=true

This is probably the point to discuss tests that I'm not intending on updating for instruction referencing and still have -experimental-debug-variable-locations=false in the arguments. They are:

MIR/X86/dvl-livedebugvalues-clobber.mir
MIR/X86/dvl-livedebugvalues-join.mir
MIR/X86/dvl-livedebugvalues-movements.mir
MIR/X86/dvl-livedebugvalues-spillrestore.mir
X86/dbg-val-list-dangling.ll

^ they involve variadic variable locations, which is currently "future work" for December,

MIR/X86/kill-after-spill.mir
MIR/X86/live-debug-values-restore-collide.mir
MIR/X86/mlicm-hoist-post-regalloc.mir
X86/live-debug-variables.ll
X86/live-debug-vars-discard-invalid.mir
X86/live-debug-vars-intervals.mir
MIR/X86/live-debug-vars-unused-arg-debugonly.mir
MIR/X86/live-debug-vars-unused-arg.mir
MIR/X86/livedebugvars-crossbb-interval.mir
MIR/X86/backup-entry-values-usage.mir

^ these all test very DBG_VALUE specific parts of code, usuallly LiveDebugVariables which is a no-op when using instruction referencing. One of them tests what VarLocBasedLDV does does with spills when they're followed by kill flags, which is another thing InstrRefBasedLDV doesn't care about.

Generic/linear-dbg-value.ll

^ this seems to just check that DBG_VALUE instructions are generated, I don't think it's really testing any interesting behaviours.

Diff Detail

Event Timeline

jmorse created this revision.Nov 26 2021, 4:21 AM
jmorse requested review of this revision.Nov 26 2021, 4:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 26 2021, 4:21 AM
jmorse updated this revision to Diff 390003.Nov 26 2021, 4:21 AM

(Don't test for cc1 flag that's being deleted)

Driver flag change seems ok but would appreciate another pair of eyes checking it makes sense. Everything else (test change omissions, new test, default flag settings) LGTM.

llvm/lib/Target/X86/X86TargetMachine.cpp
29

nit: Is this change necessary?

jmorse added inline comments.Nov 30 2021, 4:24 AM
llvm/lib/Target/X86/X86TargetMachine.cpp
29

Nope, will strip that out.

djtodoro accepted this revision.Nov 30 2021, 4:36 AM

lgtm, thanks!

This revision is now accepted and ready to land.Nov 30 2021, 4:36 AM

Cheers all, especially for the latest reviews; we'll now see whether there are extra edge cases I haven't caught!

Hmmm. I thought this was quiet after landing -- and it turns out the reason why is there's still a sneek-circuit in clang that's turning instruction referencing off by default. Woe is me; should have done more end-to-end testing :|. On the upside, in the last few weeks LTO and non-clang frontends have had instruction referencing on, so it hasn't been completely worthless.

I'm going to drop in a patch removing the bit of clang that's turning this off again, on the "obvious fix" principle that that's what this patch was supposed to do. I'll also include in the commit message that ideally rG3c045070882f should be reverted rather than the follow-up.