[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

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

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.

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("disable-post-ra"))
+    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/test/CodeGen/X86/suppress-post-ra.ll

+; Check if the function attribute "disable-post-ra" suppresses the post-RA scheduler.
+; Note that we ignore the first line of the print-before and print-after output,
+; which should be the only difference.
+;
+; RUN: llc < %s -O2 -print-before=post-RA-sched 2>&1 > /dev/null - | tail -n +2 > %t1
+; RUN: llc < %s -O2 -print-after=post-RA-sched 2>&1 > /dev/null - | tail -n +2 > %t2
+; RUN: diff %t1 %t2
+;
+;
+; ModuleID = 't.cpp'
+
+@data = global i32 17, align 4
+@sum = global i32 0, align 4
+@zero = global i32 0, align 4
+@ptr = global i32* null, align 8
+
+define i32 @main() #0 {
+entry:
+  %val = alloca i32, align 4
+  %0 = bitcast i32* %val to i8*
+  call void @llvm.lifetime.start(i64 4, i8* %0) #3
+  %1 = load i32, i32* @data, align 4, !tbaa !2
+  store i32 %1, i32* %val, align 4, !tbaa !2
+  call void @_Z3fooiPi(i32 1, i32* nonnull %val) #3
+  call void @_Z3fooiPi(i32 2, i32* nonnull @data) #3
+  %2 = load i32, i32* @zero, align 4, !tbaa !2
+  call void @llvm.lifetime.end(i64 4, i8* %0) #3
+  %3 = load i32, i32* %val, align 4, !tbaa !2
+  call void (...) @llvm.fake.use(i32 %3)
+  ret i32 %2
+}
+
+declare void @llvm.lifetime.start(i64, i8* nocapture) #1
+
+declare void @_Z3fooiPi(i32, i32*)
+
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+
+declare void @llvm.fake.use(...)
+
+attributes #0 = { "disable-post-ra" }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.8.1"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"int", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}