Skip to content

Commit

Permalink
ARM: Fix CMP_SWAP expansion
Browse files Browse the repository at this point in the history 
Clean up after my misguided attempt in r304267 to "fix" CMP_SWAP
returning an uninitialized status value.

- I was always using tMOVi8 to zero the status register which cannot
  encode higher register numbers and llvm would silently miscompile)

- Nobody was ever looking at that status value outside the expansion.
  ARMDAGToDAGISel::SelectCMP_SWAP() the only place creating CMP_SWAP
  instructions was not mapping anything to it. (The cmpxchg status value
  from llvm IR is lowered to a manual comparison after the CMP_SWAP)

So this:
- Renames the register from "status" to "temp" it make it obvious that
  it isn't used outside the expansion.
- Remove the zeroing status/temp register.
- Keep the live-in list improvements from r304267

Fixes http://llvm.org/PR34056

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@310534 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@MatzeB
MatzeB committed Aug 9, 2017
1 parent 21784bb commit 566237c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 38 deletions.
38 changes: 11 additions & 27 deletions lib/Target/ARM/ARMExpandPseudoInsts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,7 @@ bool ARMExpandPseudo::ExpandCMP_SWAP(MachineBasicBlock &MBB,
MachineInstr &MI = *MBBI;
DebugLoc DL = MI.getDebugLoc();
const MachineOperand &Dest = MI.getOperand(0);
unsigned StatusReg = MI.getOperand(1).getReg();
bool StatusDead = MI.getOperand(1).isDead();
unsigned TempReg = MI.getOperand(1).getReg();
// Duplicating undef operands into 2 instructions does not guarantee the same
// value on both; However undef should be replaced by xzr anyway.
assert(!MI.getOperand(2).isUndef() && "cannot handle undef");
Expand All @@ -790,23 +789,9 @@ bool ARMExpandPseudo::ExpandCMP_SWAP(MachineBasicBlock &MBB,
}

// .Lloadcmp:
// mov wStatus, #0
// ldrex rDest, [rAddr]
// cmp rDest, rDesired
// bne .Ldone
if (!StatusDead) {
if (IsThumb) {
BuildMI(LoadCmpBB, DL, TII->get(ARM::tMOVi8), StatusReg)
.addDef(ARM::CPSR, RegState::Dead)
.addImm(0)
.add(predOps(ARMCC::AL));
} else {
BuildMI(LoadCmpBB, DL, TII->get(ARM::MOVi), StatusReg)
.addImm(0)
.add(predOps(ARMCC::AL))
.add(condCodeOp());
}
}

MachineInstrBuilder MIB;
MIB = BuildMI(LoadCmpBB, DL, TII->get(LdrexOp), Dest.getReg());
Expand All @@ -829,10 +814,10 @@ bool ARMExpandPseudo::ExpandCMP_SWAP(MachineBasicBlock &MBB,
LoadCmpBB->addSuccessor(StoreBB);

// .Lstore:
// strex rStatus, rNew, [rAddr]
// cmp rStatus, #0
// strex rTempReg, rNew, [rAddr]
// cmp rTempReg, #0
// bne .Lloadcmp
MIB = BuildMI(StoreBB, DL, TII->get(StrexOp), StatusReg)
MIB = BuildMI(StoreBB, DL, TII->get(StrexOp), TempReg)
.addReg(NewReg)
.addReg(AddrReg);
if (StrexOp == ARM::t2STREX)
Expand All @@ -841,7 +826,7 @@ bool ARMExpandPseudo::ExpandCMP_SWAP(MachineBasicBlock &MBB,

unsigned CMPri = IsThumb ? ARM::t2CMPri : ARM::CMPri;
BuildMI(StoreBB, DL, TII->get(CMPri))
.addReg(StatusReg, getKillRegState(StatusDead))
.addReg(TempReg, RegState::Kill)
.addImm(0)
.add(predOps(ARMCC::AL));
BuildMI(StoreBB, DL, TII->get(Bcc))
Expand Down Expand Up @@ -897,8 +882,7 @@ bool ARMExpandPseudo::ExpandCMP_SWAP_64(MachineBasicBlock &MBB,
MachineInstr &MI = *MBBI;
DebugLoc DL = MI.getDebugLoc();
MachineOperand &Dest = MI.getOperand(0);
unsigned StatusReg = MI.getOperand(1).getReg();
bool StatusDead = MI.getOperand(1).isDead();
unsigned TempReg = MI.getOperand(1).getReg();
// Duplicating undef operands into 2 instructions does not guarantee the same
// value on both; However undef should be replaced by xzr anyway.
assert(!MI.getOperand(2).isUndef() && "cannot handle undef");
Expand All @@ -924,7 +908,7 @@ bool ARMExpandPseudo::ExpandCMP_SWAP_64(MachineBasicBlock &MBB,
// .Lloadcmp:
// ldrexd rDestLo, rDestHi, [rAddr]
// cmp rDestLo, rDesiredLo
// sbcs rStatus<dead>, rDestHi, rDesiredHi
// sbcs rTempReg<dead>, rDestHi, rDesiredHi
// bne .Ldone
unsigned LDREXD = IsThumb ? ARM::t2LDREXD : ARM::LDREXD;
MachineInstrBuilder MIB;
Expand Down Expand Up @@ -952,17 +936,17 @@ bool ARMExpandPseudo::ExpandCMP_SWAP_64(MachineBasicBlock &MBB,
LoadCmpBB->addSuccessor(StoreBB);

// .Lstore:
// strexd rStatus, rNewLo, rNewHi, [rAddr]
// cmp rStatus, #0
// strexd rTempReg, rNewLo, rNewHi, [rAddr]
// cmp rTempReg, #0
// bne .Lloadcmp
unsigned STREXD = IsThumb ? ARM::t2STREXD : ARM::STREXD;
MIB = BuildMI(StoreBB, DL, TII->get(STREXD), StatusReg);
MIB = BuildMI(StoreBB, DL, TII->get(STREXD), TempReg);
addExclusiveRegPair(MIB, New, 0, IsThumb, TRI);
MIB.addReg(AddrReg).add(predOps(ARMCC::AL));

unsigned CMPri = IsThumb ? ARM::t2CMPri : ARM::CMPri;
BuildMI(StoreBB, DL, TII->get(CMPri))
.addReg(StatusReg, getKillRegState(StatusDead))
.addReg(TempReg, RegState::Kill)
.addImm(0)
.add(predOps(ARMCC::AL));
BuildMI(StoreBB, DL, TII->get(Bcc))
Expand Down
10 changes: 5 additions & 5 deletions lib/Target/ARM/ARMInstrInfo.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6060,21 +6060,21 @@ def SPACE : PseudoInst<(outs GPR:$Rd), (ins i32imm:$size, GPR:$Rn),
// significantly more naive than the standard expansion: we conservatively
// assume seq_cst, strong cmpxchg and omit clrex on failure.

let Constraints = "@earlyclobber $Rd,@earlyclobber $status",
let Constraints = "@earlyclobber $Rd,@earlyclobber $temp",
mayLoad = 1, mayStore = 1 in {
def CMP_SWAP_8 : PseudoInst<(outs GPR:$Rd, GPR:$status),
def CMP_SWAP_8 : PseudoInst<(outs GPR:$Rd, GPR:$temp),
(ins GPR:$addr, GPR:$desired, GPR:$new),
NoItinerary, []>, Sched<[]>;

def CMP_SWAP_16 : PseudoInst<(outs GPR:$Rd, GPR:$status),
def CMP_SWAP_16 : PseudoInst<(outs GPR:$Rd, GPR:$temp),
(ins GPR:$addr, GPR:$desired, GPR:$new),
NoItinerary, []>, Sched<[]>;

def CMP_SWAP_32 : PseudoInst<(outs GPR:$Rd, GPR:$status),
def CMP_SWAP_32 : PseudoInst<(outs GPR:$Rd, GPR:$temp),
(ins GPR:$addr, GPR:$desired, GPR:$new),
NoItinerary, []>, Sched<[]>;

def CMP_SWAP_64 : PseudoInst<(outs GPRPair:$Rd, GPR:$status),
def CMP_SWAP_64 : PseudoInst<(outs GPRPair:$Rd, GPR:$temp),
(ins GPR:$addr, GPRPair:$desired, GPRPair:$new),
NoItinerary, []>, Sched<[]>;
}
Expand Down
9 changes: 3 additions & 6 deletions test/CodeGen/ARM/cmpxchg-O0.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@ define { i8, i1 } @test_cmpxchg_8(i8* %addr, i8 %desired, i8 %new) nounwind {
; CHECK: dmb ish
; CHECK: uxtb [[DESIRED:r[0-9]+]], [[DESIRED]]
; CHECK: [[RETRY:.LBB[0-9]+_[0-9]+]]:
; CHECK: mov{{s?}} [[STATUS:r[0-9]+]], #0
; CHECK: ldrexb [[OLD:r[0-9]+]], [r0]
; CHECK: cmp [[OLD]], [[DESIRED]]
; CHECK: bne [[DONE:.LBB[0-9]+_[0-9]+]]
; CHECK: strexb [[STATUS]], r2, [r0]
; CHECK: strexb [[STATUS:r[0-9]+]], r2, [r0]
; CHECK: cmp{{(\.w)?}} [[STATUS]], #0
; CHECK: bne [[RETRY]]
; CHECK: [[DONE]]:
Expand All @@ -30,11 +29,10 @@ define { i16, i1 } @test_cmpxchg_16(i16* %addr, i16 %desired, i16 %new) nounwind
; CHECK: dmb ish
; CHECK: uxth [[DESIRED:r[0-9]+]], [[DESIRED]]
; CHECK: [[RETRY:.LBB[0-9]+_[0-9]+]]:
; CHECK: mov{{s?}} [[STATUS:r[0-9]+]], #0
; CHECK: ldrexh [[OLD:r[0-9]+]], [r0]
; CHECK: cmp [[OLD]], [[DESIRED]]
; CHECK: bne [[DONE:.LBB[0-9]+_[0-9]+]]
; CHECK: strexh [[STATUS]], r2, [r0]
; CHECK: strexh [[STATUS:r[0-9]+]], r2, [r0]
; CHECK: cmp{{(\.w)?}} [[STATUS]], #0
; CHECK: bne [[RETRY]]
; CHECK: [[DONE]]:
Expand All @@ -50,11 +48,10 @@ define { i32, i1 } @test_cmpxchg_32(i32* %addr, i32 %desired, i32 %new) nounwind
; CHECK: dmb ish
; CHECK-NOT: uxt
; CHECK: [[RETRY:.LBB[0-9]+_[0-9]+]]:
; CHECK: mov{{s?}} [[STATUS:r[0-9]+]], #0
; CHECK: ldrex [[OLD:r[0-9]+]], [r0]
; CHECK: cmp [[OLD]], [[DESIRED]]
; CHECK: bne [[DONE:.LBB[0-9]+_[0-9]+]]
; CHECK: strex [[STATUS]], r2, [r0]
; CHECK: strex [[STATUS:r[0-9]+]], r2, [r0]
; CHECK: cmp{{(\.w)?}} [[STATUS]], #0
; CHECK: bne [[RETRY]]
; CHECK: [[DONE]]:
Expand Down

0 comments on commit 566237c

Please sign in to comment.