[EarlyIfConverter] Fix reg killed twice after early-if-predicator and ifcvt #133554
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-backend-hexagon Author: Afanasyev Ivan (ivafanas) ChangesBug relates to Example: After early-if-predicator will become: Having Fix proposal: find registers killed in both blocks, TBB and FBB, and clear kill flags for them. Full diff: https://github.com/llvm/llvm-project/pull/133554.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 24c6dafc60459..fa9ff37e625a2 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -17,6 +17,7 @@
#include "llvm/CodeGen/EarlyIfConversion.h"
#include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SparseSet.h"
@@ -31,6 +32,7 @@
#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
#include "llvm/CodeGen/MachineRegisterInfo.h"
#include "llvm/CodeGen/MachineTraceMetrics.h"
+#include "llvm/CodeGen/Register.h"
#include "llvm/CodeGen/TargetInstrInfo.h"
#include "llvm/CodeGen/TargetRegisterInfo.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
@@ -163,6 +165,9 @@ class SSAIfConv {
/// Insert selects and rewrite PHI operands to use them.
void rewritePHIOperands();
+ /// Remove killed flag for virtual regs killed in TBB and FBB simultaniously.
+ void clearDoubleKillFlags(MachineBasicBlock *TBB, MachineBasicBlock *FBB);
+
public:
/// init - Initialize per-function data structures.
void init(MachineFunction &MF) {
@@ -675,6 +680,28 @@ void SSAIfConv::rewritePHIOperands() {
}
}
+void SSAIfConv::clearDoubleKillFlags(MachineBasicBlock *TBB,
+ MachineBasicBlock *FBB) {
+ assert(TBB != FBB);
+
+ // Collect virtual registers killed in TBB.
+ SmallDenseSet<Register> TBBKilledReg;
+ for (MachineInstr &MI : TBB->instrs()) {
+ for (MachineOperand &MO : MI.operands()) {
+ if (MO.isReg() && MO.isKill() && MO.getReg().isVirtual())
+ TBBKilledReg.insert(MO.getReg());
+ }
+ }
+
+ // Find the same killed registers in FBB and clear kill flags for them.
+ for (MachineInstr &MI : FBB->instrs()) {
+ for (MachineOperand &MO : MI.operands()) {
+ if (MO.isReg() && MO.isKill() && TBBKilledReg.contains(MO.getReg()))
+ MRI->clearKillFlags(MO.getReg());
+ }
+ }
+}
+
/// convertIf - Execute the if conversion after canConvertIf has determined the
/// feasibility.
///
@@ -690,6 +717,13 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
else
++NumDiamondsConv;
+ // If both blocks are going to be merged into Head, remove "killed" flag from
+ // registers, which are killed in TBB and FBB. Otherwise, register will be
+ // killed twice in Head after splice. Double killed register is an incorrect
+ // MIR.
+ if (TBB != Tail && FBB != Tail)
+ clearDoubleKillFlags(TBB, FBB);
+
// Move all instructions into Head, except for the terminators.
if (TBB != Tail) {
if (Predicate)
diff --git a/llvm/test/CodeGen/AArch64/early-ifcvt-on-double-killed-reg.mir b/llvm/test/CodeGen/AArch64/early-ifcvt-on-double-killed-reg.mir
new file mode 100644
index 0000000000000..bda1972165442
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/early-ifcvt-on-double-killed-reg.mir
@@ -0,0 +1,53 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=aarch64-- -run-pass=early-ifcvt -stress-early-ifcvt %s -o - -verify-machineinstrs | FileCheck %s
+
+# Test that "killed" flag on the same virtual register in merged blocks is not
+# saved. Otherwise, register will be killed twice in a single block in the
+# resulting MIR, which is incorrect.
+
+---
+name: same_def_different_operand
+tracksRegLiveness: true
+liveins:
+ - { reg: '$w0', virtual-reg: '%0' }
+body: |
+ ; CHECK-LABEL: name: same_def_different_operand
+ ; CHECK: bb.0.entry:
+ ; CHECK-NEXT: liveins: $w0
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr32common = COPY $w0
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr64common = COPY $x0
+ ; CHECK-NEXT: [[SUBSWri:%[0-9]+]]:gpr32 = SUBSWri [[COPY]], 1, 0, implicit-def $nzcv
+ ; CHECK-NEXT: [[SUBWri:%[0-9]+]]:gpr32common = SUBWri [[COPY]], 3, 0
+ ; CHECK-NEXT: [[SUBWri1:%[0-9]+]]:gpr32common = SUBWri [[COPY]], 2, 0
+ ; CHECK-NEXT: [[CSELWr:%[0-9]+]]:gpr32common = CSELWr [[SUBWri]], [[SUBWri1]], 1, implicit $nzcv
+ ; CHECK-NEXT: $x2 = COPY [[COPY1]]
+ ; CHECK-NEXT: RET_ReallyLR implicit $x2
+ bb.0.entry:
+ successors: %bb.1, %bb.2
+ liveins: $w0
+
+ %0:gpr32common = COPY $w0
+ %1:gpr64common = COPY $x0
+ %2:gpr32 = SUBSWri %0, 1, 0, implicit-def $nzcv
+ Bcc 1, %bb.2, implicit $nzcv
+ B %bb.1
+
+ bb.1:
+ successors: %bb.3
+
+ %3:gpr32common = SUBWri killed %0, 2, 0
+ B %bb.3
+
+ bb.2:
+ successors: %bb.3
+
+ %4:gpr32common = SUBWri killed %0, 3, 0
+ B %bb.3
+
+ bb.3:
+ %5:gpr32common = PHI %3, %bb.1, %4, %bb.2
+ $x2 = COPY %1
+ RET_ReallyLR implicit $x2
+
+...
diff --git a/llvm/test/CodeGen/Hexagon/early-if-predicator-reg-killed-everywhere.mir b/llvm/test/CodeGen/Hexagon/early-if-predicator-reg-killed-everywhere.mir
new file mode 100644
index 0000000000000..162fc5e5192bf
--- /dev/null
+++ b/llvm/test/CodeGen/Hexagon/early-if-predicator-reg-killed-everywhere.mir
@@ -0,0 +1,93 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=hexagon -run-pass early-if-predicator %s -o - -verify-machineinstrs | FileCheck %s
+
+# Test that "killed" flag for the same register in merged blocks is not saved.
+# Otherwise, the same register will be killed twice in a single final block.
+
+--- |
+ target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"
+
+ define void @if-cvt(ptr %p, i1 %c) {
+ entry:
+ ret void
+ }
+
+...
+---
+name: if-cvt
+alignment: 16
+exposesReturnsTwice: false
+legalized: false
+regBankSelected: false
+selected: false
+failedISel: false
+tracksRegLiveness: true
+hasWinCFI: false
+registers:
+ - { id: 0, class: intregs, preferred-register: '' }
+ - { id: 1, class: intregs, preferred-register: '' }
+ - { id: 2, class: predregs, preferred-register: '' }
+liveins:
+ - { reg: '$r0', virtual-reg: '%0' }
+ - { reg: '$r1', virtual-reg: '%1' }
+frameInfo:
+ isFrameAddressTaken: false
+ isReturnAddressTaken: false
+ hasStackMap: false
+ hasPatchPoint: false
+ stackSize: 0
+ offsetAdjustment: 0
+ maxAlignment: 0
+ adjustsStack: false
+ hasCalls: true
+ stackProtector: ''
+ maxCallFrameSize: 4294967295
+ cvBytesOfCalleeSavedRegisters: 0
+ hasOpaqueSPAdjustment: false
+ hasVAStart: false
+ hasMustTailInVarArgFunc: false
+ localFrameSize: 0
+ savePoint: ''
+ restorePoint: ''
+fixedStack: []
+stack: []
+callSites: []
+constants: []
+machineFunctionInfo: {}
+body: |
+ ; CHECK-LABEL: name: if-cvt
+ ; CHECK: bb.0:
+ ; CHECK-NEXT: liveins: $r0, $r1
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:intregs = COPY $r1
+ ; CHECK-NEXT: [[S2_tstbit_i:%[0-9]+]]:predregs = S2_tstbit_i [[COPY]], 0
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:intregs = COPY $r0
+ ; CHECK-NEXT: S4_storeirif_io [[S2_tstbit_i]], [[COPY1]], 0, 1
+ ; CHECK-NEXT: S4_storeirit_io [[S2_tstbit_i]], [[COPY1]], 0, 1
+ ; CHECK-NEXT: PS_jmpret $r31, implicit-def dead $pc
+ bb.0:
+ successors: %bb.1(0x40000000), %bb.2(0x40000000)
+ liveins: $r0, $r1
+
+ %1:intregs = COPY $r1
+ %2:predregs = S2_tstbit_i %1, 0
+ %0:intregs = COPY $r0
+ J2_jumpf %2, %bb.2, implicit-def dead $pc
+ J2_jump %bb.1, implicit-def dead $pc
+
+ bb.1:
+ successors: %bb.3(0x80000000)
+
+ S4_storeiri_io killed %0, 0, 1
+ J2_jump %bb.3, implicit-def dead $pc
+
+ bb.2:
+ successors: %bb.3(0x80000000)
+
+ S4_storeiri_io killed %0, 0, 1
+ J2_jump %bb.3, implicit-def dead $pc
+
+ bb.3:
+ PS_jmpret $r31, implicit-def dead $pc
+
+...
|
Contributor
Author
|
Please, note, that I do not have commit access. If you find this patch is ok, could you please merge the PR? |
Contributor
Author
|
@MaskRay @jroelofs @fhahn @jmmartinez Could you please review the PR? Could you also please check the correctness of transformation for Hexagon. It seems suspicious, that |
jroelofs
reviewed
Mar 29, 2025
| stack: [] | ||
| callSites: [] | ||
| constants: [] | ||
| machineFunctionInfo: {} |
Contributor
There was a problem hiding this comment.
I think most of the stuff before body: is unnecessary, and can be removed.
Contributor
Author
There was a problem hiding this comment.
Thank you for the link!
Done.
| // killed twice in Head after splice. Double killed register is an incorrect | ||
| // MIR. | ||
| if (TBB != Tail && FBB != Tail) | ||
| clearDoubleKillFlags(TBB, FBB); |
Contributor
There was a problem hiding this comment.
IIUC you need to remove kill flags from TBB as instructions from that block are spliced before those in FBB.
Contributor
Author
There was a problem hiding this comment.
I like your idea more than my original one.
Implementation is updated to conform your proposal.
Done.
ivafanas
force-pushed
the
fix_double_killed_reg_after_early_if_predicator_and_conversion
branch
from
c593d80 to
0395c2a
Compare
jmmartinez
reviewed
Mar 31, 2025
Contributor
jmmartinez
left a comment
jmmartinez
left a comment
There was a problem hiding this comment.
Just to small comments, otherwise it looks good to me
llvm/test/CodeGen/Hexagon/early-if-predicator-reg-killed-everywhere.mir
Outdated
Show resolved
Hide resolved
ivafanas
force-pushed
the
fix_double_killed_reg_after_early_if_predicator_and_conversion
branch
from
0395c2a to
8963797
Compare
jmmartinez
approved these changes
Apr 1, 2025
Contributor
Author
|
@jmmartinez , thank you. Could you please merge the PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug relates to
early-if-predicatorandearly-ifcvtpasses. If virtual register has "killed" flag in both basic blocks to be merged into head, both instructions in head basic block will have "killed" flag for this register. It makes MIR incorrect.Example:
After early-if-predicator will become:
Having
killedflag set twice in bb.0 for%0is an incorrect MIR.