[llvm] r371678 - [X86] Fix latent bugs in 32-bit CMPXCHG8B inserter

[Reid Kleckner via llvm-commits]

Author: rnk
Date: Wed Sep 11 14:56:17 2019
New Revision: 371678

URL: http://llvm.org/viewvc/llvm-project?rev=371678&view=rev
Log:
[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.

Modified:
    llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
    llvm/trunk/lib/Target/X86/X86InstrBuilder.h
    llvm/trunk/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll

Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=371678&r1=371677&r2=371678&view=diff
==============================================================================
--- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
+++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Wed Sep 11 14:56:17 2019
@@ -31592,10 +31592,14 @@ X86TargetLowering::EmitInstrWithCustomIn
     // 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);
 

Modified: llvm/trunk/lib/Target/X86/X86InstrBuilder.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrBuilder.h?rev=371678&r1=371677&r2=371678&view=diff
==============================================================================
--- llvm/trunk/lib/Target/X86/X86InstrBuilder.h (original)
+++ llvm/trunk/lib/Target/X86/X86InstrBuilder.h Wed Sep 11 14:56:17 2019
@@ -131,11 +131,11 @@ addDirectMem(const MachineInstrBuilder &
 /// 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);
 }
 

Modified: llvm/trunk/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll?rev=371678&r1=371677&r2=371678&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll (original)
+++ llvm/trunk/test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll Wed Sep 11 14:56:17 2019
@@ -33,3 +33,64 @@ define void @foo_alloca_direct_address(i
 ; 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
+
+ at 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..}})