From ff45955fc8684e5a2be22224edcc677ef6d54f5d Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 11 Sep 2019 21:56:17 +0000 Subject: [PATCH] [X86] Fix latent bugs in 32-bit CMPXCHG8B inserter I found three issues: 1. the loop over E[ABCD]X copies run over BB start 2. the direct address of cmpxchg8b could be a frame index 3. the displacement of cmpxchg8b could be a global instead of an immediate These were all introduced together in r287875, and should be fixed with this change. Issue reported by Zachary Turner. llvm-svn: 371678 --- llvm/lib/Target/X86/X86ISelLowering.cpp | 12 ++-- llvm/lib/Target/X86/X86InstrBuilder.h | 6 +- .../X86/cmpxchg8b_alloca_regalloc_handling.ll | 61 +++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp index 8cc4b5f56fe8..5a274cb3e19b 100644 --- a/llvm/lib/Target/X86/X86ISelLowering.cpp +++ b/llvm/lib/Target/X86/X86ISelLowering.cpp @@ -31592,10 +31592,14 @@ X86TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI, // After X86TargetLowering::ReplaceNodeResults CMPXCHG8B is glued to its // four operand definitions that are E[ABCD] registers. We skip them and // then insert the LEA. - MachineBasicBlock::iterator MBBI(MI); - while (MBBI->definesRegister(X86::EAX) || MBBI->definesRegister(X86::EBX) || - MBBI->definesRegister(X86::ECX) || MBBI->definesRegister(X86::EDX)) - --MBBI; + MachineBasicBlock::reverse_iterator RMBBI(MI.getReverseIterator()); + while (RMBBI != BB->rend() && (RMBBI->definesRegister(X86::EAX) || + RMBBI->definesRegister(X86::EBX) || + RMBBI->definesRegister(X86::ECX) || + RMBBI->definesRegister(X86::EDX))) { + ++RMBBI; + } + MachineBasicBlock::iterator MBBI(RMBBI); addFullAddress( BuildMI(*BB, *MBBI, DL, TII->get(X86::LEA32r), computedAddrVReg), AM); diff --git a/llvm/lib/Target/X86/X86InstrBuilder.h b/llvm/lib/Target/X86/X86InstrBuilder.h index 50aed98112c3..aa45e9b191c1 100644 --- a/llvm/lib/Target/X86/X86InstrBuilder.h +++ b/llvm/lib/Target/X86/X86InstrBuilder.h @@ -131,11 +131,11 @@ addDirectMem(const MachineInstrBuilder &MIB, unsigned Reg) { /// reference. static inline void setDirectAddressInInstr(MachineInstr *MI, unsigned Operand, unsigned Reg) { - // Direct memory address is in a form of: Reg, 1 (Scale), NoReg, 0, NoReg. - MI->getOperand(Operand).setReg(Reg); + // Direct memory address is in a form of: Reg/FI, 1 (Scale), NoReg, 0, NoReg. + MI->getOperand(Operand).ChangeToRegister(Reg, /*isDef=*/false); MI->getOperand(Operand + 1).setImm(1); MI->getOperand(Operand + 2).setReg(0); - MI->getOperand(Operand + 3).setImm(0); + MI->getOperand(Operand + 3).ChangeToImmediate(0); MI->getOperand(Operand + 4).setReg(0); } diff --git a/llvm/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll b/llvm/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll index b500484a4c89..364d78aa3144 100644 --- a/llvm/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll +++ b/llvm/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll @@ -33,3 +33,64 @@ define void @foo_alloca_direct_address(i64* %addr, i32 %n) { ; CHECK-LABEL: foo_alloca_direct_address ; CHECK-NOT: leal {{\(%e.*\)}}, [[REGISTER:%e.i]] ; CHECK: lock cmpxchg8b ([[REGISTER]]) + +; We used to have a bug when combining: +; - base pointer for stack frame (VLA + alignment) +; - cmpxchg8b frameindex + index reg + +declare void @escape(i32*) + +define void @foo_alloca_index(i32 %i, i64 %val) { +entry: + %Counters = alloca [19 x i64], align 32 + %vla = alloca i32, i32 %i + call void @escape(i32* %vla) + br label %body + +body: + %p = getelementptr inbounds [19 x i64], [19 x i64]* %Counters, i32 0, i32 %i + %t2 = cmpxchg volatile i64* %p, i64 %val, i64 %val seq_cst seq_cst + %t3 = extractvalue { i64, i1 } %t2, 0 + %cmp.i = icmp eq i64 %val, %t3 + br i1 %cmp.i, label %done, label %body + +done: + ret void +} + +; Check that we add a LEA +; CHECK-LABEL: foo_alloca_index: +; CHECK: leal {{[0-9]*\(%e..,%e..,8\), %e..}} +; CHECK: lock cmpxchg8b ({{%e..}}) + + + +; We used to have a bug when combining: +; - base pointer for stack frame (VLA + alignment) +; - cmpxchg8b global + index reg + +@Counters = external global [19 x i64] + +define void @foo_alloca_index_global(i32 %i, i64 %val) { +entry: + %aligner = alloca i32, align 32 + call void @escape(i32* %aligner) + %vla = alloca i32, i32 %i + call void @escape(i32* %vla) + br label %body + +body: + %p = getelementptr inbounds [19 x i64], [19 x i64]* @Counters, i32 0, i32 %i + %t2 = cmpxchg volatile i64* %p, i64 %val, i64 %val seq_cst seq_cst + %t3 = extractvalue { i64, i1 } %t2, 0 + %cmp.i = icmp eq i64 %val, %t3 + br i1 %cmp.i, label %done, label %body + +done: + ret void +} + +; Check that we add a LEA +; CHECK-LABEL: foo_alloca_index_global: +; CHECK: leal {{Counters\(,%e..,8\), %e..}} +; CHECK: lock cmpxchg8b ({{%e..}})