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

[jmorse]

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.

[jmorse]

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

[Orlando]

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 ↗

(On Diff #390003)

nit: Is this change necessary?

[jmorse]

llvm/lib/Target/X86/X86TargetMachine.cpp

29 ↗

(On Diff #390003)

Nope, will strip that out.

[djtodoro]

lgtm, thanks!

[jmorse]

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

[jmorse]

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.

llvm/lib/CodeGen/CommandFlags.cpp

 /// Features, and command line flags.
 void codegen::setFunctionAttributes(StringRef CPU, StringRef Features,
                                     Module &M) {
   for (Function &F : M)
     setFunctionAttributes(CPU, Features, F);
 }
 
 bool codegen::getDefaultValueTrackingVariableLocations(const llvm::Triple &T) {
+  if (T.getArch() == llvm::Triple::x86_64)
+    return true;
   return false;
 }

llvm/test/DebugInfo/X86/instr-ref-flag.ll

 ; RUN: llc %s -o - -stop-before=finalize-isel -march=x86-64 \
 ; RUN: | FileCheck %s --check-prefixes=INSTRREFON
 ; RUN: llc %s -o - -stop-before=finalize-isel -march=x86-64 \
 ; RUN:    -experimental-debug-variable-locations=true \
 ; RUN: | FileCheck %s --check-prefixes=INSTRREFON
 
 ; RUN: llc %s -o - -stop-before=finalize-isel -march=x86-64 \
 ; RUN:    -experimental-debug-variable-locations=false \
 ; RUN: | FileCheck %s --check-prefixes=INSTRREFOFF \
 ; RUN:    --implicit-check-not=DBG_INSTR_REF
 
 ;; This test checks that for an x86 triple, instruction referencing is used
 ;; by llc by default, and that it can be turned explicitly on or off as
 ;; desired.
 
-;; XFail it for pre-landing the patch, and to allow a minimal delta if it
-;; has to be reverted.
-; XFAIL: *
-
 ; INSTRREFON: DBG_INSTR_REF
 ; INSTRREFOFF: DBG_VALUE
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-unknown"
 
 define hidden i32 @foo(i32 %a) local_unnamed_addr !dbg !7 {
   %b = add i32 %a, 1