Skip to content

Commit 3fb5b18

Browse files
author
Mogball
committed
Revert 24633ea and 760e7d0 "Enable FoldImmediate for X86"
This reverts commits 24633ea and 760e7d0. I have confirmed that these commits are introducing a new crash in the peephole optimizer. I have minimized a test case, which you can find below. ```llvmir ; ModuleID = 'bugpoint-reduced-simplified.bc' source_filename = "/mnt/big/modular/Kernels/mojo/Mogg/MOGG.mojo" 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" target triple = "x86_64-unknown-linux-gnu" declare dso_local void @foo({ { ptr, [4 x i64], [4 x i64], i1 }, { ptr, [4 x i64], [4 x i64], i1 } }, { ptr }, { ptr, i64, i8 }) define dso_local void @bad_fn(ptr %0, ptr %1, ptr %2) { %4 = load i64, ptr null, align 8 %5 = insertvalue [4 x i64] poison, i64 12, 1 %6 = insertvalue [4 x i64] %5, i64 poison, 2 %7 = insertvalue [4 x i64] %6, i64 poison, 3 %8 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } poison, [4 x i64] %7, 1 %9 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } %8, [4 x i64] poison, 2 %10 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } %9, i1 poison, 3 %11 = icmp ne i64 %4, 1 %12 = or i1 false, %11 %13 = select i1 %12, i64 %4, i64 0 %14 = zext i1 %12 to i64 %15 = insertvalue [4 x i64] poison, i64 12, 1 %16 = insertvalue [4 x i64] %15, i64 poison, 2 %17 = insertvalue [4 x i64] %16, i64 %13, 3 %18 = insertvalue [4 x i64] poison, i64 %14, 3 %19 = icmp eq i64 0, 0 %20 = icmp eq i64 0, 0 %21 = icmp eq i64 %13, 0 %22 = and i1 %20, %19 %23 = select i1 %22, i1 %21, i1 false %24 = select i1 %23, i1 %12, i1 false %25 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } poison, [4 x i64] %17, 1 %26 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } %25, [4 x i64] %18, 2 %27 = insertvalue { ptr, [4 x i64], [4 x i64], i1 } %26, i1 %24, 3 %28 = insertvalue { { ptr, [4 x i64], [4 x i64], i1 }, { ptr, [4 x i64], [4 x i64], i1 } } undef, { ptr, [4 x i64], [4 x i64], i1 } %10, 0 %29 = insertvalue { { ptr, [4 x i64], [4 x i64], i1 }, { ptr, [4 x i64], [4 x i64], i1 } } %28, { ptr, [4 x i64], [4 x i64], i1 } %27, 1 br label %31 30: ; preds = %3 br label %softmax_pass 31: ; preds = %31 %exitcond.not.i = icmp eq i64 poison, 3 br i1 %exitcond.not.i, label %37, label %31 32: ; preds = %31 br i1 poison, label %34, label %33 33: ; preds = %32 br label %34 34: ; preds = %33, %32 br i1 poison, label %35, label %36 35: ; preds = %34 br label %softmax_pass 36: ; preds = %34 br i1 poison, label %37, label %.critedge.i 37: ; preds = %36 br i1 poison, label %38, label %.critedge.i 38: ; preds = %37 br i1 poison, label %40, label %39 39: ; preds = %38 br label %40 40: ; preds = %39, %38 br i1 poison, label %.lr.ph28.i, label %._crit_edge.i .lr.ph28.i: ; preds = %40 br label %41 41: ; preds = %51, %.lr.ph28.i br i1 poison, label %.thread, label %42 42: ; preds = %41 br i1 poison, label %43, label %44 43: ; preds = %42 br label %45 44: ; preds = %42 br label %45 45: ; preds = %44, %43 br i1 poison, label %46, label %.thread 46: ; preds = %45 br label %47 .thread: ; preds = %45, %41 br label %47 47: ; preds = %.thread, %46 br i1 poison, label %51, label %48 48: ; preds = %47 br i1 poison, label %49, label %50 49: ; preds = %48 br label %51 50: ; preds = %48 br label %51 51: ; preds = %50, %49, %47 call void @foo({ { ptr, [4 x i64], [4 x i64], i1 }, { ptr, [4 x i64], [4 x i64], i1 } } %29, { ptr } poison, { ptr, i64, i8 } poison) br i1 poison, label %._crit_edge.i, label %41 ._crit_edge.i: ; preds = %51, %40 br label %softmax_pass .critedge.i: ; preds = %37, %36 br i1 poison, label %.lr.ph.i, label %softmax_pass .lr.ph.i: ; preds = %.lr.ph.i, %.critedge.i store { ptr, [4 x i64], [4 x i64], i1 } %10, ptr poison, align 8 br i1 poison, label %.lr.ph.i, label %softmax_pass softmax_pass: ; preds = %.lr.ph.i, %.critedge.i, %._crit_edge.i, %35, %30 ret void } ```
1 parent 3fe2be7 commit 3fb5b18

21 files changed

+2210
-2910
lines changed

llvm/lib/CodeGen/PeepholeOptimizer.cpp

Lines changed: 16 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,7 @@ namespace {
202202
bool isMoveImmediate(MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
203203
DenseMap<Register, MachineInstr *> &ImmDefMIs);
204204
bool foldImmediate(MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
205-
DenseMap<Register, MachineInstr *> &ImmDefMIs,
206-
bool &Deleted);
205+
DenseMap<Register, MachineInstr *> &ImmDefMIs);
207206

208207
/// Finds recurrence cycles, but only ones that formulated around
209208
/// a def operand and a use operand that are tied. If there is a use
@@ -218,11 +217,8 @@ namespace {
218217
/// set \p CopyMIs. If this virtual register was previously seen as a
219218
/// copy, replace the uses of this copy with the previously seen copy's
220219
/// destination register.
221-
/// \p LocalMIs contains all previous seen instructions. An optimized away
222-
/// instruction should be deleted from LocalMIs.
223220
bool foldRedundantCopy(MachineInstr &MI,
224-
DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs,
225-
SmallPtrSetImpl<MachineInstr *> &LocalMIs);
221+
DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs);
226222

227223
/// Is the register \p Reg a non-allocatable physical register?
228224
bool isNAPhysCopy(Register Reg);
@@ -1355,28 +1351,26 @@ bool PeepholeOptimizer::isMoveImmediate(
13551351
MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
13561352
DenseMap<Register, MachineInstr *> &ImmDefMIs) {
13571353
const MCInstrDesc &MCID = MI.getDesc();
1358-
if (MCID.getNumDefs() != 1 || !MI.getOperand(0).isReg())
1354+
if (!MI.isMoveImmediate())
13591355
return false;
1360-
Register Reg = MI.getOperand(0).getReg();
1361-
if (!Reg.isVirtual())
1362-
return false;
1363-
1364-
int64_t ImmVal;
1365-
if (!MI.isMoveImmediate() && !TII->getConstValDefinedInReg(MI, Reg, ImmVal))
1356+
if (MCID.getNumDefs() != 1)
13661357
return false;
1358+
Register Reg = MI.getOperand(0).getReg();
1359+
if (Reg.isVirtual()) {
1360+
ImmDefMIs.insert(std::make_pair(Reg, &MI));
1361+
ImmDefRegs.insert(Reg);
1362+
return true;
1363+
}
13671364

1368-
ImmDefMIs.insert(std::make_pair(Reg, &MI));
1369-
ImmDefRegs.insert(Reg);
1370-
return true;
1365+
return false;
13711366
}
13721367

13731368
/// Try folding register operands that are defined by move immediate
13741369
/// instructions, i.e. a trivial constant folding optimization, if
13751370
/// and only if the def and use are in the same BB.
13761371
bool PeepholeOptimizer::foldImmediate(
13771372
MachineInstr &MI, SmallSet<Register, 4> &ImmDefRegs,
1378-
DenseMap<Register, MachineInstr *> &ImmDefMIs, bool &Deleted) {
1379-
Deleted = false;
1373+
DenseMap<Register, MachineInstr *> &ImmDefMIs) {
13801374
for (unsigned i = 0, e = MI.getDesc().getNumOperands(); i != e; ++i) {
13811375
MachineOperand &MO = MI.getOperand(i);
13821376
if (!MO.isReg() || MO.isDef())
@@ -1390,19 +1384,6 @@ bool PeepholeOptimizer::foldImmediate(
13901384
assert(II != ImmDefMIs.end() && "couldn't find immediate definition");
13911385
if (TII->FoldImmediate(MI, *II->second, Reg, MRI)) {
13921386
++NumImmFold;
1393-
// FoldImmediate can delete ImmDefMI if MI was its only user. If ImmDefMI
1394-
// is not deleted, and we happened to get a same MI, we can delete MI and
1395-
// replace its users.
1396-
if (MRI->getVRegDef(Reg) &&
1397-
MI.isIdenticalTo(*II->second, MachineInstr::IgnoreVRegDefs)) {
1398-
Register DstReg = MI.getOperand(0).getReg();
1399-
if (DstReg.isVirtual() &&
1400-
MRI->getRegClass(DstReg) == MRI->getRegClass(Reg)) {
1401-
MRI->replaceRegWith(DstReg, Reg);
1402-
MI.eraseFromParent();
1403-
Deleted = true;
1404-
}
1405-
}
14061387
return true;
14071388
}
14081389
}
@@ -1424,8 +1405,7 @@ bool PeepholeOptimizer::foldImmediate(
14241405
//
14251406
// Should replace %2 uses with %1:sub1
14261407
bool PeepholeOptimizer::foldRedundantCopy(
1427-
MachineInstr &MI, DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs,
1428-
SmallPtrSetImpl<MachineInstr *> &LocalMIs) {
1408+
MachineInstr &MI, DenseMap<RegSubRegPair, MachineInstr *> &CopyMIs) {
14291409
assert(MI.isCopy() && "expected a COPY machine instruction");
14301410

14311411
Register SrcReg = MI.getOperand(1).getReg();
@@ -1445,10 +1425,6 @@ bool PeepholeOptimizer::foldRedundantCopy(
14451425
}
14461426

14471427
MachineInstr *PrevCopy = CopyMIs.find(SrcPair)->second;
1448-
// A COPY instruction can be deleted or changed by other optimizations.
1449-
// Check if the previous COPY instruction is existing and still a COPY.
1450-
if (!LocalMIs.count(PrevCopy) || !PrevCopy->isCopy())
1451-
return false;
14521428

14531429
assert(SrcSubReg == PrevCopy->getOperand(1).getSubReg() &&
14541430
"Unexpected mismatching subreg!");
@@ -1756,7 +1732,7 @@ bool PeepholeOptimizer::runOnMachineFunction(MachineFunction &MF) {
17561732
continue;
17571733
}
17581734

1759-
if (MI->isCopy() && (foldRedundantCopy(*MI, CopySrcMIs, LocalMIs) ||
1735+
if (MI->isCopy() && (foldRedundantCopy(*MI, CopySrcMIs) ||
17601736
foldRedundantNAPhysCopy(*MI, NAPhysToVirtMIs))) {
17611737
LocalMIs.erase(MI);
17621738
LLVM_DEBUG(dbgs() << "Deleting redundant copy: " << *MI << "\n");
@@ -1774,14 +1750,8 @@ bool PeepholeOptimizer::runOnMachineFunction(MachineFunction &MF) {
17741750
// next iteration sees the new instructions.
17751751
MII = MI;
17761752
++MII;
1777-
if (SeenMoveImm) {
1778-
bool Deleted;
1779-
Changed |= foldImmediate(*MI, ImmDefRegs, ImmDefMIs, Deleted);
1780-
if (Deleted) {
1781-
LocalMIs.erase(MI);
1782-
continue;
1783-
}
1784-
}
1753+
if (SeenMoveImm)
1754+
Changed |= foldImmediate(*MI, ImmDefRegs, ImmDefMIs);
17851755
}
17861756

17871757
// Check whether MI is a load candidate for folding into a later

0 commit comments

Comments
 (0)