-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[X86] Resolve FIXME: Copy kill flag for eflags #82411
Conversation
@llvm/pr-subscribers-llvm-regalloc @llvm/pr-subscribers-backend-x86 Author: AtariDreams (AtariDreams) ChangesWe now mark the last eflags usage as kill if the def was also kill. Full diff: https://github.com/llvm/llvm-project/pull/82411.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86FlagsCopyLowering.cpp b/llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
index af25b34fbab995..e9830f086668a0 100644
--- a/llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
+++ b/llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
@@ -683,6 +683,8 @@ bool X86FlagsCopyLoweringPass::runOnMachineFunction(MachineFunction &MF) {
// Now rewrite the jumps that use the flags. These we handle specially
// because if there are multiple jumps in a single basic block we'll have
// to do surgery on the CFG.
+ bool CopyDefIsKill = CopyDefI.killsRegister(X86::EFLAGS);
+ MachineOperand *LastEflagsUse = nullptr;
MachineBasicBlock *LastJmpMBB = nullptr;
for (MachineInstr *JmpI : JmpIs) {
// Past the first jump within a basic block we need to split the blocks
@@ -693,10 +695,13 @@ bool X86FlagsCopyLoweringPass::runOnMachineFunction(MachineFunction &MF) {
LastJmpMBB = JmpI->getParent();
rewriteCondJmp(*TestMBB, TestPos, TestLoc, *JmpI, CondRegs);
+ if (JmpI->readsRegister(X86::EFLAGS))
+ LastEflagsUse = JmpI->findRegisterUseOperand(X86::EFLAGS);
}
- // FIXME: Mark the last use of EFLAGS before the copy's def as a kill if
- // the copy's def operand is itself a kill.
+ // After the loop that processes jumps:
+ if (LastEflagsUse && CopyDefIsKill)
+ LastEflagsUse->setIsKill(true);
}
#ifndef NDEBUG
|
|
Can you write an MIR test for this? |
89107da
to
903afee
Compare
c17b2ac
to
f5fda25
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
bae0d51
to
2078401
Compare
2683066
to
9f837c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should we consider altering the findRegisterDefOperandIdx/findRegisterDefOperand argument order and require the TRI pointer to help avoid this in the future?
I do think this actually. |
dc06306
to
b5ea7ec
Compare
@AtariDreams any luck with adding test coverage? |
We now mark the last eflags usage as kill if the def was also kill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs test coverage - and the patch title/description needs updating to actually describe what that patch does and is for
Fixes #82659 There are some functions, such as `findRegisterDefOperandIdx` and `findRegisterDefOperand`, that have too many default parameters. As a result, we have encountered some issues due to the lack of TRI parameters, as shown in issue #82411. Following @RKSimon 's suggestion, this patch refactors 9 functions, including `{reads, kills, defines, modifies}Register`, `registerDefIsDead`, and `findRegister{UseOperandIdx, UseOperand, DefOperandIdx, DefOperand}`, adjusting the order of the TRI parameter and making it required. In addition, all the places that call these functions have also been updated correctly to ensure no additional impact. After this, the caller of these functions should explicitly know whether to pass the `TargetRegisterInfo` or just a `nullptr`.
We now mark the last eflags usage as kill if the def was also kill.