[ExtendLifetimes][1/4] Add "disable-post-ra" function attribute to disable the post-regalloc scheduler

[StephenTozer]

Summary

This patch is part of a set of patches that add an -fextend-lifetimes flag to clang, which extends the lifetimes of local variables and parameters for improved debuggability. In addition to that flag, the patch series adds a new function attribute to disable the post-RA scheduler (which is applied by -fextend-lifetimes), a pragma to selectively disable -fextend-lifetimes, and an -fextend-this-ptr flag which functions as -fextend-lifetimes for this pointers only. All changes and tests in these patches were written by @wolfgangp, though I will be managing these reviews and addressing any comments raised. Discussion on the approach of this patch series as a whole should be directed to the 3rd patch, D157613, in order to simplify review.

---

This particular patch implements the disable-post-ra function attribute, which trivially stops the post-register-allocation scheduler from running. This is useful when using the -fextend-lifetimes flag, as post-RA scheduling is not directly affected by that flag. The reason for creating an attribute instead of simply disabling the post-RA scheduler with a flag is that -fextend-lifetimes may be enabled or disabled for individual functions, and we only want to skip scheduling for the functions which desire extended lifetimes.

Although this patch is technically unrelated to debug info, I am bringing in debug info reviewers because of how this patch connects to the following patches in the sequence.

[Orlando]

llvm/lib/CodeGen/PostRASchedulerList.cpp

282

bikeshed: disable-post-ra seems potentially unclear. Perhaps disable-post-ra-sched?

llvm/test/CodeGen/X86/suppress-post-ra.ll

7 ↗

(On Diff #549017)

Do you need to run all the O2 passes? Can we not just use --run-pass=post-RA-sched?

Secondly, IMO it'd be good to check that post-RA-sched does something to the input when the attribute isn't present to avoid a rotten green test. You could probably achieve that with a sed command to add/remove the attribute (see example in dbg-empty-metadata-lowering.ll).

21 ↗

(On Diff #549017)

Can the tbaa metadata and lifetime markers be removed?

[StephenTozer]

llvm/lib/CodeGen/PostRASchedulerList.cpp

282

SGTM in principle, but it's worth noting that the flag to disable the post-RA scheduler is -disable-post-ra, so there's an argument that it would be more consistent to make the attribute and command line flag with identical functionality have the same name. I'm not attached to either side though, what do you think?

[jmorse]

Do we need to register this attribute somewhere, in a .td file or something, to ensure it's discoverable by other people performing maintenance?

As we're adding an attribute, https://www.youtube.com/watch?v=6bmlqtjSPFw comes to mind ('2021 LLVM Dev Mtg "Please Stop Adding Attributes"'), but it sounds like we're not covered by that argument, not the least because we're not changing visible behaviours. Possibly a (bikeshed warning) more abstract name like "prioritize-debuggability" that's then interpreted by the post-ra scheduler would be better. No strong opinion from me.

Otherwise lgtm with Orlando's comments addressed (this should run just the single pass ideally)

[Orlando]

llvm/lib/CodeGen/PostRASchedulerList.cpp

282

Ah, since there's precedent I think it's better to be consistent, so stick with it IMO (unless the name if changed to something more generic as per Jeremy's suggestion).

[dblaikie]

I think the discourse thread speculated about an 'optdebug' attribute?

[StephenTozer]

In D157615#4634448, @jmorse wrote:

Do we need to register this attribute somewhere, in a .td file or something, to ensure it's discoverable by other people performing maintenance?

Yeah, I think there are a few missing spots in this patch for documentation and bitcode representation that I'll fill in now.

Possibly a (bikeshed warning) more abstract name like "prioritize-debuggability" that's then interpreted by the post-ra scheduler would be better. No strong opinion from me.

Bikeshedding concerns accepted given the discussion on the current discourse RFC - an attribute like prioritize-debuggability or opt-debug makes sense, since that also helps solve the "passing optimization info through LTO" problem.

[StephenTozer]

Significant patch updates: Changed attribute from "disable-post-ra" to optdebug, following the direction that the discussion on the Og RFC appears to be taking. In the course of this it's been updated to be a keyword rather than an arbitrary string, which has added some bulk to the patch.

[dblaikie]

Might be worth rewording the commit, or splitting it - I'd say the introduction of optdebug should be the noteworthier part of this patch (or whichever patch introduces it) - so either "this patch adds optdebug, and a first/exemplar use of it in postra scheduler" or split it into a patch that just adds the attribute and no uses, and one that adds the use. (I'd lean towards splitting it up, personally & I guess maybe moving to github pull requests in the process, perhaps)

[StephenTozer]

In D157615#4646848, @dblaikie wrote:

Might be worth rewording the commit, or splitting it - I'd say the introduction of optdebug should be the noteworthier part of this patch (or whichever patch introduces it) - so either "this patch adds optdebug, and a first/exemplar use of it in postra scheduler" or split it into a patch that just adds the attribute and no uses, and one that adds the use.

SGTM, the scope of this patch has increased as-of the latest update. That being said AFAIK Github PRs don't currently have support for stacked PRs, so I imagine I'll need to have one review up at a time.

[dblaikie]

In D157615#4647235, @StephenTozer wrote:

In D157615#4646848, @dblaikie wrote:

Might be worth rewording the commit, or splitting it - I'd say the introduction of optdebug should be the noteworthier part of this patch (or whichever patch introduces it) - so either "this patch adds optdebug, and a first/exemplar use of it in postra scheduler" or split it into a patch that just adds the attribute and no uses, and one that adds the use.

SGTM, the scope of this patch has increased as-of the latest update. That being said AFAIK Github PRs don't currently have support for stacked PRs, so I imagine I'll need to have one review up at a time.

Yeah, don't think we have a clear path for stacked PRs right now - so, yeah, will try to work through them one at a time. I guess you could post PRs that contain all the commits (eg: one review containing commit A, another review containing A and B, etc) - it doesn't have a good solution if large changes to predecessor patches need updates to latter patches (eg: if A causes invasive changes to B) - but otherwise it's probably OK-ish... (& in this case I don't think any changes to "adding optdebug" are likely to substantially change the nature of subsequent patches). But maybe not a big deal either way/can just wait for the subsequent patches.

clang/lib/CodeGen/CodeGenModule.cpp

if ((ShouldAddOptNone || D->hasAttr<OptimizeNoneAttr>()) &&
     B.addAttribute(llvm::Attribute::NoInline);
 
     // We still need to handle naked functions even though optnone subsumes
     // much of their semantics.
     if (D->hasAttr<NakedAttr>())
       B.addAttribute(llvm::Attribute::Naked);
 
     // OptimizeNone wins over OptimizeForSize and MinSize.
+    F->removeFnAttr(llvm::Attribute::OptimizeForDebugging);
     F->removeFnAttr(llvm::Attribute::OptimizeForSize);
     F->removeFnAttr(llvm::Attribute::MinSize);
   } else if (D->hasAttr<NakedAttr>()) {
     // Naked implies noinline: we should not be inlining such functions.
     B.addAttribute(llvm::Attribute::Naked);
     B.addAttribute(llvm::Attribute::NoInline);
   } else if (D->hasAttr<NoDuplicateAttr>()) {
     B.addAttribute(llvm::Attribute::NoDuplicate);

llvm/docs/BitCodeFormat.rst

 * code 69: ``byref``
 * code 70: ``mustprogress``
 * code 74: ``vscale_range(<Min>[, <Max>])``
 * code 75: ``swiftasync``
 * code 76: ``nosanitize_coverage``
 * code 77: ``elementtype``
 * code 78: ``disable_sanitizer_instrumentation``
 * code 79: ``nosanitize_bounds``
+* code 88: ``optdebug``
 
 .. note::
   The ``allocsize`` attribute has a special encoding for its arguments. Its two
   arguments, which are 32-bit integers, are packed into one 64-bit integer value
   (i.e. ``(EltSizeParam << 32) | NumEltsParam``), with ``NumEltsParam`` taking on
   the sentinel value -1 if it is not specified.
 
 .. note::

llvm/docs/LangRef.rst

 ``null_pointer_is_valid``
    If ``null_pointer_is_valid`` is set, then the ``null`` address
    in address-space 0 is considered to be a valid address for memory loads and
    stores. Any analysis or optimization should not treat dereferencing a
    pointer to ``null`` as undefined behavior in this function.
    Note: Comparing address of a global variable to ``null`` may still
    evaluate to false because of a limitation in querying this attribute inside
    constant expressions.
+``optdebug``
+    This attribute suggests that optimization passes and code generator passes
+    should make choices that try to preserve debug info without significantly
+    degrading runtime performance.
 ``optforfuzzing``
     This attribute indicates that this function should be optimized
     for maximum fuzzing signal.
 ``optnone``
     This function attribute indicates that most optimization passes will skip
     this function, with the exception of interprocedural optimization passes.
     Code generation defaults to the "fast" instruction selector.
     This attribute cannot be used together with the ``alwaysinline``

llvm/include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_ALLOC_ALIGN = 80,
   ATTR_KIND_ALLOCATED_POINTER = 81,
   ATTR_KIND_ALLOC_KIND = 82,
   ATTR_KIND_PRESPLIT_COROUTINE = 83,
   ATTR_KIND_FNRETTHUNK_EXTERN = 84,
   ATTR_KIND_SKIP_PROFILE = 85,
   ATTR_KIND_MEMORY = 86,
   ATTR_KIND_NOFPCLASS = 87,
+  ATTR_KIND_OPTIMIZE_FOR_DEBUGGING = 88,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

llvm/include/llvm/IR/Attributes.td

 def NoSanitizeBounds : EnumAttr<"nosanitize_bounds", [FnAttr]>;
 
 /// No SanitizeCoverage instrumentation.
 def NoSanitizeCoverage : EnumAttr<"nosanitize_coverage", [FnAttr]>;
 
 /// Null pointer in address space zero is valid.
 def NullPointerIsValid : EnumAttr<"null_pointer_is_valid", [FnAttr]>;
 
+/// Select optimizations that give decent debug info.
+def OptimizeForDebugging : EnumAttr<"optdebug", [FnAttr]>;
+
 /// Select optimizations for best fuzzing signal.
 def OptForFuzzing : EnumAttr<"optforfuzzing", [FnAttr]>;
 
 /// opt_size.
 def OptimizeForSize : EnumAttr<"optsize", [FnAttr]>;
 
 /// Function must not be optimized.
 def OptimizeNone : EnumAttr<"optnone", [FnAttr]>;

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_NO_UNWIND:
     return Attribute::NoUnwind;
   case bitc::ATTR_KIND_NO_SANITIZE_BOUNDS:
     return Attribute::NoSanitizeBounds;
   case bitc::ATTR_KIND_NO_SANITIZE_COVERAGE:
     return Attribute::NoSanitizeCoverage;
   case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
     return Attribute::NullPointerIsValid;
+  case bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING:
+    return Attribute::OptimizeForDebugging;
   case bitc::ATTR_KIND_OPT_FOR_FUZZING:
     return Attribute::OptForFuzzing;
   case bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE:
     return Attribute::OptimizeForSize;
   case bitc::ATTR_KIND_OPTIMIZE_NONE:
     return Attribute::OptimizeNone;
   case bitc::ATTR_KIND_READ_NONE:
     return Attribute::ReadNone;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::NoUnwind:
     return bitc::ATTR_KIND_NO_UNWIND;
   case Attribute::NoSanitizeBounds:
     return bitc::ATTR_KIND_NO_SANITIZE_BOUNDS;
   case Attribute::NoSanitizeCoverage:
     return bitc::ATTR_KIND_NO_SANITIZE_COVERAGE;
   case Attribute::NullPointerIsValid:
     return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;
+  case Attribute::OptimizeForDebugging:
+    return bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING;
   case Attribute::OptForFuzzing:
     return bitc::ATTR_KIND_OPT_FOR_FUZZING;
   case Attribute::OptimizeForSize:
     return bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE;
   case Attribute::OptimizeNone:
     return bitc::ATTR_KIND_OPTIMIZE_NONE;
   case Attribute::ReadNone:
     return bitc::ATTR_KIND_READ_NONE;

llvm/lib/CodeGen/PostRASchedulerList.cpp

bool PostRAScheduler::enablePostRAScheduler(
   return ST.enablePostRAScheduler() &&
          OptLevel >= ST.getOptLevelToEnablePostRAScheduler();
 }
 
 bool PostRAScheduler::runOnMachineFunction(MachineFunction &Fn) {
   if (skipFunction(Fn.getFunction()))
     return false;
 
+  if (Fn.getFunction().hasFnAttribute(Attribute::OptimizeForDebugging))
+    return false;
+
   TII = Fn.getSubtarget().getInstrInfo();
   MachineLoopInfo &MLI = getAnalysis<MachineLoopInfo>();
   AliasAnalysis *AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   TargetPassConfig *PassConfig = &getAnalysis<TargetPassConfig>();
 
   RegClassInfo.runOnMachineFunction(Fn);
 
   TargetSubtargetInfo::AntiDepBreakMode AntiDepMode =

llvm/lib/Transforms/Utils/CodeExtractor.cpp

if (Attr.isStringAttribute()) {
       case Attribute::NoImplicitFloat:
       case Attribute::NoInline:
       case Attribute::NonLazyBind:
       case Attribute::NoRedZone:
       case Attribute::NoUnwind:
       case Attribute::NoSanitizeBounds:
       case Attribute::NoSanitizeCoverage:
       case Attribute::NullPointerIsValid:
+      case Attribute::OptimizeForDebugging:
       case Attribute::OptForFuzzing:
       case Attribute::OptimizeNone:
       case Attribute::OptimizeForSize:
       case Attribute::SafeStack:
       case Attribute::ShadowCallStack:
       case Attribute::SanitizeAddress:
       case Attribute::SanitizeMemory:
       case Attribute::SanitizeThread:

llvm/test/Bitcode/attributes.ll

 }
 
 ; CHECK: define void @f87() [[FNRETTHUNKEXTERN:#[0-9]+]]
 define void @f87() fn_ret_thunk_extern { ret void }
 
 ; CHECK: define void @f88() [[SKIPPROFILE:#[0-9]+]]
 define void @f88() skipprofile { ret void }
 
+define void @f89() optdebug
+; CHECK: define void @f89() [[OPTDEBUG:#[0-9]+]]
+{
+        ret void;
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { memory(none) }
 ; CHECK: attributes #3 = { memory(read) }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

 ; CHECK: attributes #47 = { vscale_range(1,0) }
 ; CHECK: attributes #48 = { nosanitize_coverage }
 ; CHECK: attributes #49 = { noprofile }
 ; CHECK: attributes #50 = { disable_sanitizer_instrumentation }
 ; CHECK: attributes #51 = { uwtable(sync) }
 ; CHECK: attributes #52 = { nosanitize_bounds }
 ; CHECK: attributes [[FNRETTHUNKEXTERN]] = { fn_ret_thunk_extern }
 ; CHECK: attributes [[SKIPPROFILE]] = { skipprofile }
+; CHECK: attributes [[OPTDEBUG]] = { optdebug }
 ; CHECK: attributes #[[NOBUILTIN]] = { nobuiltin }

llvm/test/CodeGen/X86/suppress-post-ra.mir

+# Check if the function attribute "optdebug" suppresses the post-RA scheduler.
+
+# Note: This code is a reduced sample of code that will be affected by the
+# post-RA scheduler, and the test will start to fail if the post-RA scheduler
+# no longer makes any change to this test case; if this starts to happen due to
+# a valid post-RA scheduler change, either change the test so that it is still
+# affected, or XFAIL the test.
+
+# RUN: llc %s -run-pass verify -o %t1
+# RUN: llc %s -run-pass post-RA-sched -o %t2
+# RUN: diff %t1 %t2
+
+# Detect if this test becomes rotten by verifying that post-RA-sched does
+# *something* to this test case.
+# RUN: sed 's/optdebug //' %s | llc - -x mir -run-pass verify -o %t3
+# RUN: sed 's/optdebug //' %s | llc - -x mir -run-pass post-RA-sched -o %t4
+# RUN: not diff %t3 %t4
+
+--- |
+  ; ModuleID = '/tmp/llvm-reduce-efe8f0.ll'
+  source_filename = "/tmp/llvm-reduce-efe8f0.ll"
+  target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
+  target triple = "i686-unknown-unknown"
+  
+  define void @update() #0 {
+  entry:
+    %v = alloca <3 x i16>, i32 0, align 8
+    ret void
+  }
+  
+  attributes #0 = { optdebug "target-cpu"="slm" }
+
+...
+---
+name:            update
+alignment:       16
+tracksRegLiveness: true
+tracksDebugUserValues: true
+registers:       []
+liveins:         []
+frameInfo:
+  stackSize:       12
+  offsetAdjustment: -8
+  maxAlignment:    8
+  stackProtector:  ''
+  functionContext: ''
+  maxCallFrameSize: 0
+  cvBytesOfCalleeSavedRegisters: 0
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:
+  - { id: 0, type: spill-slot, offset: -8, size: 4, alignment: 4, stack-id: default, 
+      callee-saved-register: '', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+stack:
+  - { id: 0, name: v, type: default, offset: -16, size: 1, alignment: 8, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+entry_values:    []
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    frame-setup PUSH32r killed $ebp, implicit-def $esp, implicit $esp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    frame-setup CFI_INSTRUCTION offset $ebp, -8
+    $ebp = frame-setup MOV32rr $esp
+    frame-setup CFI_INSTRUCTION def_cfa_register $ebp
+    $esp = frame-setup AND32ri $esp, -8, implicit-def dead $eflags
+    $esp = MOV32rr $ebp
+    $ebp = frame-destroy POP32r implicit-def $esp, implicit $esp
+    frame-destroy CFI_INSTRUCTION def_cfa $esp, 4
+    RET32
+
+...

llvm/utils/emacs/llvm-mode.el

 
 (defvar llvm-font-lock-keywords
   (list
    ;; Attributes
    `(,(regexp-opt
        '("alwaysinline" "argmemonly" "allocsize" "builtin" "cold" "convergent" "dereferenceable" "dereferenceable_or_null" "hot" "immarg" "inaccessiblememonly"
          "inaccessiblemem_or_argmemonly" "inalloca" "inlinehint" "jumptable" "minsize" "mustprogress" "naked" "nobuiltin" "nonnull" "nocapture"
          "nocallback" "nocf_check" "noduplicate" "nofree" "noimplicitfloat" "noinline" "nomerge" "nonlazybind" "noprofile" "noredzone" "noreturn"
-         "norecurse" "nosync" "noundef" "nounwind" "nosanitize_bounds" "nosanitize_coverage" "null_pointer_is_valid" "optforfuzzing" "optnone" "optsize" "preallocated" "readnone" "readonly" "returned" "returns_twice"
+         "norecurse" "nosync" "noundef" "nounwind" "nosanitize_bounds" "nosanitize_coverage" "null_pointer_is_valid" "optdebug" "optforfuzzing" "optnone" "optsize" "preallocated" "readnone" "readonly" "returned" "returns_twice"
          "shadowcallstack" "signext" "speculatable" "speculative_load_hardening" "ssp" "sspreq" "sspstrong" "safestack" "sanitize_address" "sanitize_hwaddress" "sanitize_memtag"
          "sanitize_thread" "sanitize_memory" "strictfp" "swifterror" "uwtable" "vscale_range" "willreturn" "writeonly" "zeroext") 'symbols) . font-lock-constant-face)
    ;; Variables
    '("%[-a-zA-Z$._][-a-zA-Z$._0-9]*" . font-lock-variable-name-face)
    ;; Labels
    '("[-a-zA-Z$._0-9]+:" . font-lock-variable-name-face)
    ;; Unnamed variable slots
    '("%[-]?[0-9]+" . font-lock-variable-name-face)

llvm/utils/kate/llvm.xml

<list name="function-attributes">
       <item> noinline </item>
       <item> nomerge </item>
       <item> noprofile </item>
       <item> noredzone </item>
       <item> noreturn </item>
       <item> nosync </item>
       <item> nounwind </item>
       <item> null_pointer_is_valid </item>
+      <item> optdebug </item>
       <item> optforfuzzing </item>
       <item> optnone </item>
       <item> optsize </item>
       <item> preallocated </item>
       <item> readnone </item>
       <item> readonly </item>
       <item> safestack </item>
       <item> sanitize_hwaddress </item>

llvm/utils/vim/syntax/llvm.vim

syn keyword llvmKeyword
       \ noredzone
       \ noreturn
       \ nosync
       \ noundef
       \ nounwind
       \ nosanitize_bounds
       \ nosanitize_coverage
       \ null_pointer_is_valid
+      \ optdebug
       \ optforfuzzing
       \ optnone
       \ optsize
       \ personality
       \ preallocated
       \ private
       \ protected
       \ ptx_device