[LLVM] Add HasFakeUses to MachineFunction #110097
Conversation
Following the addition of the llvm.fake.use intrinsic and corresponding MIR instruction, two further changes are planned: to add an -fextend-lifetimes flag to Clang that emits these intrinsics, and to have -Og enable this flag by default. Currently, some logic for handling fake uses is gated by the optdebug attribute, which is intended to be switched on by -fextend-lifetimes (and by extension -Og later on). However, the decision was made that a general optdebug attribute should be incompatible with other opt_ attributes (e.g. optsize, optnone), since they all express different intents for how to optimize the program. We would still like to allow -fextend-lifetimes with optsize however (i.e. -Os -fextend-lifetimes should be legal), since it may be a useful configuration and there is no technical reason to not allow it.
llvmbot
added
backend:X86
llvm:globalisel
llvm:SelectionDAG
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-x86 Author: Stephen Tozer (SLTozer) ChangesFollowing the addition of the llvm.fake.use intrinsic and corresponding MIR instruction, two further changes are planned: to add an -fextend-lifetimes flag to Clang that emits these intrinsics, and to have -Og enable this flag by default. Currently, some logic for handling fake uses is gated by the optdebug attribute, which is intended to be switched on by -fextend-lifetimes (and by extension -Og later on). However, the decision was made that a general optdebug attribute should be incompatible with other opt_ attributes (e.g. optsize, optnone), since they all express different intents for how to optimize the program. We would still like to allow -fextend-lifetimes with optsize however (i.e. -Os -fextend-lifetimes should be legal), since it may be a useful configuration and there is no technical reason to not allow it. This patch resolves this by tracking MachineFunctions that have fake uses, allowing us to run passes that interact with them and skip passes that clash with them. Full diff: https://github.com/llvm/llvm-project/pull/110097.diff 12 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/MIRYamlMapping.h b/llvm/include/llvm/CodeGen/MIRYamlMapping.h
index ab8dc442e04b7b..db052dffa93cbc 100644
--- a/llvm/include/llvm/CodeGen/MIRYamlMapping.h
+++ b/llvm/include/llvm/CodeGen/MIRYamlMapping.h
@@ -740,6 +740,7 @@ struct MachineFunction {
bool HasEHCatchret = false;
bool HasEHScopes = false;
bool HasEHFunclets = false;
+ bool HasFakeUses = false;
bool IsOutlined = false;
bool FailsVerification = false;
@@ -786,6 +787,7 @@ template <> struct MappingTraits<MachineFunction> {
YamlIO.mapOptional("hasEHCatchret", MF.HasEHCatchret, false);
YamlIO.mapOptional("hasEHScopes", MF.HasEHScopes, false);
YamlIO.mapOptional("hasEHFunclets", MF.HasEHFunclets, false);
+ YamlIO.mapOptional("hasFakeUses", MF.HasFakeUses, false);
YamlIO.mapOptional("isOutlined", MF.IsOutlined, false);
YamlIO.mapOptional("debugInstrRef", MF.UseDebugInstrRef, false);
diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h
index 5c1da4fa762e84..81589246f7b5da 100644
--- a/llvm/include/llvm/CodeGen/MachineFunction.h
+++ b/llvm/include/llvm/CodeGen/MachineFunction.h
@@ -376,6 +376,7 @@ class LLVM_ABI MachineFunction {
bool HasEHCatchret = false;
bool HasEHScopes = false;
bool HasEHFunclets = false;
+ bool HasFakeUses = false;
bool IsOutlined = false;
/// BBID to assign to the next basic block of this function.
@@ -1200,6 +1201,9 @@ class LLVM_ABI MachineFunction {
bool hasEHFunclets() const { return HasEHFunclets; }
void setHasEHFunclets(bool V) { HasEHFunclets = V; }
+ bool hasFakeUses() const { return HasFakeUses; }
+ void setHasFakeUses(bool V) { HasFakeUses = V; }
+
bool isOutlined() const { return IsOutlined; }
void setIsOutlined(bool V) { IsOutlined = V; }
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 8e860a1f740295..c2132f8e742196 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -2199,6 +2199,7 @@ bool IRTranslator::translateKnownIntrinsic(const CallInst &CI, Intrinsic::ID ID,
for (auto VReg : getOrCreateVRegs(*Arg))
VRegs.push_back(VReg);
MIRBuilder.buildInstr(TargetOpcode::FAKE_USE, {}, VRegs);
+ MF->setHasFakeUses(true);
return true;
}
case Intrinsic::dbg_declare: {
diff --git a/llvm/lib/CodeGen/MIRParser/MIRParser.cpp b/llvm/lib/CodeGen/MIRParser/MIRParser.cpp
index be07fbf478b1d8..c9ec07d302e094 100644
--- a/llvm/lib/CodeGen/MIRParser/MIRParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIRParser.cpp
@@ -525,6 +525,7 @@ MIRParserImpl::initializeMachineFunction(const yaml::MachineFunction &YamlMF,
MF.setHasEHCatchret(YamlMF.HasEHCatchret);
MF.setHasEHScopes(YamlMF.HasEHScopes);
MF.setHasEHFunclets(YamlMF.HasEHFunclets);
+ MF.setHasFakeUses(YamlMF.HasFakeUses);
MF.setIsOutlined(YamlMF.IsOutlined);
if (YamlMF.Legalized)
diff --git a/llvm/lib/CodeGen/MIRPrinter.cpp b/llvm/lib/CodeGen/MIRPrinter.cpp
index cf6122bce22364..d52c1d831267f6 100644
--- a/llvm/lib/CodeGen/MIRPrinter.cpp
+++ b/llvm/lib/CodeGen/MIRPrinter.cpp
@@ -207,6 +207,7 @@ void MIRPrinter::print(const MachineFunction &MF) {
YamlMF.HasEHCatchret = MF.hasEHCatchret();
YamlMF.HasEHScopes = MF.hasEHScopes();
YamlMF.HasEHFunclets = MF.hasEHFunclets();
+ YamlMF.HasFakeUses = MF.hasFakeUses();
YamlMF.IsOutlined = MF.isOutlined();
YamlMF.UseDebugInstrRef = MF.useDebugInstrRef();
diff --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp
index 9b2862de22b690..729fdb473c8449 100644
--- a/llvm/lib/CodeGen/MachineScheduler.cpp
+++ b/llvm/lib/CodeGen/MachineScheduler.cpp
@@ -477,7 +477,7 @@ bool MachineScheduler::runOnMachineFunction(MachineFunction &mf) {
}
bool PostMachineScheduler::runOnMachineFunction(MachineFunction &mf) {
- if (skipFunction(mf.getFunction()))
+ if (mf.hasFakeUses() || skipFunction(mf.getFunction()))
return false;
if (EnablePostRAMachineSched.getNumOccurrences()) {
diff --git a/llvm/lib/CodeGen/PostRASchedulerList.cpp b/llvm/lib/CodeGen/PostRASchedulerList.cpp
index 2f7cfdd275b4fd..66f2f86e7f8321 100644
--- a/llvm/lib/CodeGen/PostRASchedulerList.cpp
+++ b/llvm/lib/CodeGen/PostRASchedulerList.cpp
@@ -275,7 +275,7 @@ bool PostRAScheduler::enablePostRAScheduler(
}
bool PostRAScheduler::runOnMachineFunction(MachineFunction &Fn) {
- if (skipFunction(Fn.getFunction()))
+ if (Fn.hasFakeUses() || skipFunction(Fn.getFunction()))
return false;
TII = Fn.getSubtarget().getInstrInfo();
diff --git a/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp b/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp
index 232181a199b8c2..ef7a58670c3ac5 100644
--- a/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp
+++ b/llvm/lib/CodeGen/RemoveLoadsIntoFakeUses.cpp
@@ -74,10 +74,8 @@ INITIALIZE_PASS_END(RemoveLoadsIntoFakeUses, DEBUG_TYPE,
"Remove Loads Into Fake Uses", false, false)
bool RemoveLoadsIntoFakeUses::runOnMachineFunction(MachineFunction &MF) {
- // Only `optdebug` functions should contain FAKE_USEs, so don't try to run
- // this for other functions.
- if (!MF.getFunction().hasFnAttribute(Attribute::OptimizeForDebugging) ||
- skipFunction(MF.getFunction()))
+ // Only run this for functions that have fake uses.
+ if (!MF.hasFakeUses() || skipFunction(MF.getFunction()))
return false;
bool AnyChanges = false;
diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
index 8405ba9ac326cf..c075e30011b64c 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
@@ -31,6 +31,7 @@
#include "llvm/IR/Function.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/Module.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/ErrorHandling.h"
@@ -200,12 +201,16 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
}
}
}
- // Look for calls to the @llvm.va_start intrinsic. We can omit some
- // prologue boilerplate for variadic functions that don't examine their
- // arguments.
if (const auto *II = dyn_cast<IntrinsicInst>(&I)) {
+ // Look for calls to the @llvm.va_start intrinsic. We can omit some
+ // prologue boilerplate for variadic functions that don't examine
+ // their arguments.
if (II->getIntrinsicID() == Intrinsic::vastart)
MF->getFrameInfo().setHasVAStart(true);
+ // Look for llvm.fake.uses, so that we can prevent certain
+ // optimizations if they are present.
+ else if (II->getIntrinsicID() == Intrinsic::fake_use)
+ MF->setHasFakeUses(true);
}
// If we have a musttail call in a variadic function, we need to ensure
diff --git a/llvm/test/CodeGen/X86/fake-use-postra-scheduler.mir b/llvm/test/CodeGen/X86/fake-use-postra-scheduler.mir
new file mode 100644
index 00000000000000..1f0f5cfa22400e
--- /dev/null
+++ b/llvm/test/CodeGen/X86/fake-use-postra-scheduler.mir
@@ -0,0 +1,55 @@
+# Prevent the post-RA machine scheduler from running on functions marked as having fake uses.
+# RUN: llc --post-RA-scheduler -run-pass post-RA-sched -mtriple=x86_64-unknown-linux --print-before=post-RA-sched --debug-only=post-RA-sched %s -o - 2>&1 >/dev/null | FileCheck %s --check-prefix=CHECK-POSTRA
+# RUN: llc --enable-post-misched=true -run-pass postmisched -mtriple=x86_64-unknown-linux --debug-only=machine-scheduler %s -o - 2>&1 >/dev/null | FileCheck %s --check-prefix=CHECK-POSTMACHINE
+# REQUIRES: asserts
+#
+# We make sure that functions that have hasFakeUses=true are not processed by
+# either of the post-regalloc scheduling passes.
+#
+# CHECK-POSTRA: Machine code for function withFakeUse
+# CHECK-POSTRA-NOT: PostRAScheduler
+# CHECK-POSTRA: Machine code for function withoutFakeUse
+# CHECK-POSTRA: PostRAScheduler
+#
+# CHECK-POSTMACHINE-NOT: Before post-MI-sched
+# CHECK-POSTMACHINE: Before post-MI-sched
+# CHECK-POSTMACHINE: Machine code for function withoutFakeUse
+#
+--- |
+ ; ModuleID = 'test.ll'
+ source_filename = "test.ll"
+ target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+
+ define void @withFakeUse() {
+ entry:
+ ret void
+ }
+
+ define void @withoutFakeUse() {
+ entry:
+ ret void
+ }
+
+...
+---
+name: withFakeUse
+alignment: 16
+hasFakeUses: true
+tracksRegLiveness: true
+registers:
+body: |
+ bb.0.entry:
+ RET 0
+
+...
+---
+name: withoutFakeUse
+alignment: 16
+hasFakeUses: false
+tracksRegLiveness: true
+registers:
+body: |
+ bb.0.entry:
+ RET 0
+
+...
diff --git a/llvm/test/tools/llvm-reduce/mir/preserve-func-info.mir b/llvm/test/tools/llvm-reduce/mir/preserve-func-info.mir
index f735dfd5cbbf01..d55a11a07f1731 100644
--- a/llvm/test/tools/llvm-reduce/mir/preserve-func-info.mir
+++ b/llvm/test/tools/llvm-reduce/mir/preserve-func-info.mir
@@ -22,6 +22,7 @@
# RESULT-NEXT: hasEHCatchret: true
# RESULT-NEXT: hasEHScopes: true
# RESULT-NEXT: hasEHFunclets: true
+# RESULT-NEXT: hasFakeUses: true
# RESULT-NEXT: failsVerification: true
# RESULT-NEXT: tracksDebugUserValues: true
@@ -54,6 +55,7 @@ callsUnwindInit: true
hasEHCatchret: true
hasEHScopes: true
hasEHFunclets: true
+hasFakeUses: true
body: |
bb.0:
diff --git a/llvm/tools/llvm-reduce/ReducerWorkItem.cpp b/llvm/tools/llvm-reduce/ReducerWorkItem.cpp
index 5409b6dc7459d3..5f6a22cb3a9741 100644
--- a/llvm/tools/llvm-reduce/ReducerWorkItem.cpp
+++ b/llvm/tools/llvm-reduce/ReducerWorkItem.cpp
@@ -403,6 +403,7 @@ static std::unique_ptr<MachineFunction> cloneMF(MachineFunction *SrcMF,
DstMF->setHasEHCatchret(SrcMF->hasEHCatchret());
DstMF->setHasEHScopes(SrcMF->hasEHScopes());
DstMF->setHasEHFunclets(SrcMF->hasEHFunclets());
+ DstMF->setHasFakeUses(SrcMF->hasFakeUses());
DstMF->setIsOutlined(SrcMF->isOutlined());
if (!SrcMF->getLandingPads().empty() ||
|
| @@ -477,7 +477,7 @@ bool MachineScheduler::runOnMachineFunction(MachineFunction &mf) { | |||
| } | |||
|
|
|||
| bool PostMachineScheduler::runOnMachineFunction(MachineFunction &mf) { | |||
| if (skipFunction(mf.getFunction())) | |||
| if (mf.hasFakeUses() || skipFunction(mf.getFunction())) | |||
There was a problem hiding this comment.
Both of these scheduler changes are surprising to me, and incorrect. We rely on the post RA schedulers to handle hazards
There was a problem hiding this comment.
That seems like a good reason to keep it. The disabling of post RA scheduling for -fextend-lifetimes is a fairly old feature for our downstream branch, and since then enough has changed (particularly the instruction-referencing LiveDebugValues implementation) that post-RA scheduling no longer appears to cause as many problems as before. I'll investigate to see whether enabling post-RA scheduling with -fextend-lifetimes downstream causes any major debug info regressions, which would necessitate modifying post-RA-sched to respect fake uses in some way, but I suspect that this is no longer needed.
There was a problem hiding this comment.
Although actually this looks like another existing bug with optnone. in AMDGPU we add a dedicated pass to recognize hazards at -O0 to replace the postRA scheduler hazard handling. That won't happen for higher opt levels for an optnone function
| if (const auto *II = dyn_cast<IntrinsicInst>(&I)) { | ||
| // Look for calls to the @llvm.va_start intrinsic. We can omit some | ||
| // prologue boilerplate for variadic functions that don't examine | ||
| // their arguments. |
There was a problem hiding this comment.
Turn this into a switch over the ID?
| # CHECK-POSTMACHINE: Before post-MI-sched | ||
| # CHECK-POSTMACHINE: Machine code for function withoutFakeUse | ||
| # | ||
| --- | |
There was a problem hiding this comment.
Don't need the IR section in this test
| tracksRegLiveness: true | ||
| registers: | ||
| body: | | ||
| bb.0.entry: |
There was a problem hiding this comment.
| bb.0.entry: | |
| bb.0: |
| tracksRegLiveness: true | ||
| registers: | ||
| body: | | ||
| bb.0.entry: |
There was a problem hiding this comment.
| bb.0.entry: | |
| bb.0: |
| body: | | ||
| bb.0.entry: | ||
| RET 0 | ||
|
|
There was a problem hiding this comment.
Should test the inference of the MIR parser. It should be able to infer hasFakeUses when the field isn't mentioned and there is a FAKE_USE instruction present
| @@ -525,6 +525,7 @@ MIRParserImpl::initializeMachineFunction(const yaml::MachineFunction &YamlMF, | |||
| MF.setHasEHCatchret(YamlMF.HasEHCatchret); | |||
| MF.setHasEHScopes(YamlMF.HasEHScopes); | |||
| MF.setHasEHFunclets(YamlMF.HasEHFunclets); | |||
| MF.setHasFakeUses(YamlMF.HasFakeUses); | |||
There was a problem hiding this comment.
Missing detection of this from the instruction stream, like we do for no-vregs and is-ssa
There was a problem hiding this comment.
The current approach for this has the property being set purely by ISel and not updated afterwards, such that we could have a function with "hasFakeUses" set which once contained FAKE_USE instructions, but where they have since been deleted by some form of DCE; that shouldn't generally happen, since FAKE_USEs are meant to be preserved, but if there are cases where they exist in an unreachable block then it's possible. In those cases, under the current logic we would still assume fake uses exist and treat the function as such.
Changing this to have inference would require us to either have a circumstance where serializing and deserializing the MachineFunction changes the result (as we detect HasFakeUses=false), have serializing and deserializing result in an error due to inconsistency, or requiring every pass that could remove all FAKE_USE instructions to be aware of them and updating the flag if needed. I think of these options, it would probably be best to change the flag to "MayHaveFakeUses" or something along those lines and just keeping track of it from ISel onwards; there should be very few circumstances where FAKE_USEs are deleted post-ISel, and none at all where they're added (so we could still check in the verifier that no fn with MayHaveFakeUses=false contains fake uses). Does that sound reasonable?
There was a problem hiding this comment.
MIR only exists as a testing mechanism, and the detection is a quality of life improvement. Concerns about passes setting this or not are moot, this is for handwritten test cases
I've read this description twice and I still don't understand why it needs a MachineFunction flag. What kind of things will be gated on it? Is it just a performance thing, so that you have a fast way to find out whether the function contains any fake uses, or does it carry more semantic information than that? |
The former - there's an additional pass that will iterate over every Instruction to find and remove loads under certain conditions which are only relevant to functions with fake uses in them, which will only appear in specific developer builds (using the |
OK. I'm ambivalent about this. On one hand it seems like there's a lot of boilerplate just for adding this flag (serializing it and deserializing it and checking for it consistency) which is all just useless complexity. On the other hand, if it saves some passes from scanning the IR of the function, that's 0.1% compile time each time. |
|
If you remove the scheduler changes then that only leaves one place where the flag is tested, in |
There are no plans for more uses right now; I think the flag would be worth keeping as it stands though. There is a moderate amount of complexity to serializing/deserializing MachineFunction flags, but this complexity already exists for the other flags (there is no novel logic in this patch), and as best I can tell we aren't incurring any significant costs for this in normal builds. It seems to me like this flag would essentially be invisible to end users and LLVM developers in all cases where they aren't working specifically on FAKE_USE-related features. On the other hand, compile time is something that developers and end users are acutely aware of, and even for small increases there ought to be a good justification for accepting it - especially since the performance cost of removing this and just always running the RemoveLoadsIntoFakeUses pass is not insignificant (compile time tracker results). |
OK. I won't object. |
|
I don't think it's an issue to track this, but it needs the auto detection |
Agreed on the auto detection, I've just been verifying it's fine for post-RA scheduling to be enabled with fake uses, which looks to be the case - added the property inference and enabled post-RA scheduling in the latest update. |
|
Merged - the commit accidentally included a HasFakeUses MachineFunctionProperty, which has been removed in an NFC followup b01be72. |
Following the addition of the llvm.fake.use intrinsic and corresponding MIR instruction, two further changes are planned: to add an -fextend-lifetimes flag to Clang that emits these intrinsics, and to have -Og enable this flag by default. Currently, some logic for handling fake uses is gated by the optdebug attribute, which is intended to be switched on by -fextend-lifetimes (and by extension -Og later on). However, the decision was made that a general optdebug attribute should be incompatible with other opt_ attributes (e.g. optsize, optnone), since they all express different intents for how to optimize the program. We would still like to allow -fextend-lifetimes with optsize however (i.e. -Os -fextend-lifetimes should be legal), since it may be a useful configuration and there is no technical reason to not allow it.
This patch resolves this by tracking MachineFunctions that have fake uses, allowing us to run passes that interact with them and skip passes that clash with them.