[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

nit: Is this change necessary?

[jmorse]

llvm/lib/Target/X86/X86TargetMachine.cpp

29

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.

clang/include/clang/Driver/Options.td

 defm lto_unit : BoolOption<"f", "lto-unit",
   CodeGenOpts<"LTOUnit">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Emit IR to support LTO unit features (CFI, whole program vtable opt)">,
   NegFlag<SetFalse>>;
 defm debug_pass_manager : BoolOption<"f", "debug-pass-manager",
   CodeGenOpts<"DebugPassManager">, DefaultFalse,
   PosFlag<SetTrue, [], "Prints debug information for the new pass manager">,
   NegFlag<SetFalse, [], "Disables debug printing for the new pass manager">>;
-def fexperimental_debug_variable_locations : Flag<["-"],
-    "fexperimental-debug-variable-locations">,
-    HelpText<"Use experimental new value-tracking variable locations">,
-    MarshallingInfoFlag<CodeGenOpts<"ValueTrackingVariableLocations">>;
 def fverify_debuginfo_preserve
     : Flag<["-"], "fverify-debuginfo-preserve">,
       HelpText<"Enable Debug Info Metadata preservation testing in "
                "optimizations.">,
       MarshallingInfoFlag<CodeGenOpts<"EnableDIPreservationVerify">>;
 def fverify_debuginfo_preserve_export
     : Joined<["-"], "fverify-debuginfo-preserve-export=">,
       MetaVarName<"<file>">,

llvm/include/llvm/CodeGen/CommandFlags.h

 
 bool getEmitCallSiteInfo();
 
 bool getEnableMachineFunctionSplitter();
 
 bool getEnableDebugEntryValues();
 
 bool getValueTrackingVariableLocations();
+Optional<bool> getExplicitValueTrackingVariableLocations();
 
 bool getForceDwarfFrameSection();
 
 bool getXRayOmitFunctionIndex();
 
 bool getDebugStrictDwarf();
 
 unsigned getAlignLoops();

 
 /// Set function attributes of function \p F based on CPU, Features, and command
 /// line flags.
 void setFunctionAttributes(StringRef CPU, StringRef Features, Function &F);
 
 /// Set function attributes of functions in Module M based on CPU,
 /// Features, and command line flags.
 void setFunctionAttributes(StringRef CPU, StringRef Features, Module &M);
+
+/// Should value-tracking variable locations / instruction referencing be
+/// enabled by default for this triple?
+bool getDefaultValueTrackingVariableLocations(const llvm::Triple &T);
 } // namespace codegen
 } // namespace llvm
 
 #endif // LLVM_CODEGEN_COMMANDFLAGS_H

llvm/lib/CodeGen/CommandFlags.cpp

 CGOPT(bool, UniqueBasicBlockSectionNames)
 CGOPT(EABI, EABIVersion)
 CGOPT(DebuggerKind, DebuggerTuningOpt)
 CGOPT(bool, EnableStackSizeSection)
 CGOPT(bool, EnableAddrsig)
 CGOPT(bool, EmitCallSiteInfo)
 CGOPT(bool, EnableMachineFunctionSplitter)
 CGOPT(bool, EnableDebugEntryValues)
-CGOPT(bool, ValueTrackingVariableLocations)
+CGOPT_EXP(bool, ValueTrackingVariableLocations)
 CGOPT(bool, ForceDwarfFrameSection)
 CGOPT(bool, XRayOmitFunctionIndex)
 CGOPT(bool, DebugStrictDwarf)
 CGOPT(unsigned, AlignLoops)
 
 codegen::RegisterCodeGenFlags::RegisterCodeGenFlags() {
 #define CGBINDOPT(NAME)                                                        \
   do {                                                                         \
codegen::InitTargetOptionsFromCodeGenFlags(const Triple &TheTriple) {
   Options.EmulatedTLS = getEmulatedTLS();
   Options.ExplicitEmulatedTLS = EmulatedTLSView->getNumOccurrences() > 0;
   Options.ExceptionModel = getExceptionModel();
   Options.EmitStackSizeSection = getEnableStackSizeSection();
   Options.EnableMachineFunctionSplitter = getEnableMachineFunctionSplitter();
   Options.EmitAddrsig = getEnableAddrsig();
   Options.EmitCallSiteInfo = getEmitCallSiteInfo();
   Options.EnableDebugEntryValues = getEnableDebugEntryValues();
-  Options.ValueTrackingVariableLocations = getValueTrackingVariableLocations();
   Options.ForceDwarfFrameSection = getForceDwarfFrameSection();
   Options.XRayOmitFunctionIndex = getXRayOmitFunctionIndex();
   Options.DebugStrictDwarf = getDebugStrictDwarf();
   Options.LoopAlignment = getAlignLoops();
 
+  if (auto Opt = getExplicitValueTrackingVariableLocations())
+    Options.ValueTrackingVariableLocations = *Opt;
+  else
+    Options.ValueTrackingVariableLocations =
+        getDefaultValueTrackingVariableLocations(TheTriple);
+
   Options.MCOptions = mc::InitMCTargetOptionsFromFlags();
 
   Options.ThreadModel = getThreadModel();
   Options.EABIVersion = getEABIVersion();
   Options.DebuggerTuning = getDebuggerTuningOpt();
   Options.SwiftAsyncFramePointer = getSwiftAsyncFramePointer();
   return Options;
 }

 
 /// Set function attributes of functions in Module M based on CPU,
 /// 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/lib/Target/X86/X86TargetMachine.cpp

 #include "X86TargetObjectFile.h"
 #include "X86TargetTransformInfo.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/CodeGen/CommandFlags.h"
 #include "llvm/CodeGen/ExecutionDomainFix.h"
 #include "llvm/CodeGen/GlobalISel/CallLowering.h"
 #include "llvm/CodeGen/GlobalISel/IRTranslator.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelect.h"
 #include "llvm/CodeGen/GlobalISel/Legalizer.h"
 #include "llvm/CodeGen/GlobalISel/RegBankSelect.h"
 #include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/CodeGen/Passes.h"

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.
+
+; 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
+  call void @llvm.dbg.value(metadata i32 %b, metadata !11, metadata !DIExpression()), !dbg !12
+  ret i32 %b, !dbg !12
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "foo.cpp", directory: ".")
+!2 = !DIBasicType(name: "int", size: 8, encoding: DW_ATE_signed)
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 6, type: !8, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!2, !2}
+!10 = !{!11}
+!11 = !DILocalVariable(name: "baz", scope: !7, file: !1, line: 7, type: !2)
+!12 = !DILocation(line: 10, scope: !7)