ARM isel bug fix for adds/subs operands.

Modified ARMISelLowering::AdjustInstrPostInstrSelection to handle the full gamut of CPSR defs/uses including instructins whose "optional" cc_out operand is not really optional. This allowed removal of the hasPostISelHook to simplify the .td files and make the implementation more robust. Fixes rdar://10137436: sqlite3 miscompile git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@140134 91177308-0d34-0410-b5e6-96231b3b80d8
m-labs · Sep 20, 2011 · 4815d56 · 4815d56
1 parent 3af7a67
commit 4815d56
Show file tree

Hide file tree

Showing 10 changed files with 120 additions and 44 deletions.
diff --git a/include/llvm/MC/MCInstrDesc.h b/include/llvm/MC/MCInstrDesc.h
@@ -477,14 +477,6 @@ class MCInstrDesc {
     return Flags & (1 << MCID::UsesCustomInserter);
   }
 
-  /// hasPostISelHook - Return true if this instruction requires *adjustment*
-  /// after instruction selection by calling a target hook. For example, this
-  /// can be used to fill in ARM 's' optional operand depending on whether
-  /// the conditional flag register is used.
-  bool hasPostISelHook() const {
-    return Flags & (1 << MCID::HasPostISelHook);
-  }
-
   /// isRematerializable - Returns true if this instruction is a candidate for
   /// remat.  This flag is deprecated, please don't use it anymore.  If this
   /// flag is set, the isReallyTriviallyReMaterializable() method is called to

diff --git a/lib/CodeGen/SelectionDAG/InstrEmitter.cpp b/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
@@ -763,8 +763,7 @@ EmitMachineNode(SDNode *Node, bool IsClone, bool IsCloned,
     }
 
   // Run post-isel target hook to adjust this instruction if needed.
-  if (II.hasPostISelHook())
-    TLI->AdjustInstrPostInstrSelection(MI, Node);
+  TLI->AdjustInstrPostInstrSelection(MI, Node);
 }
 
 /// EmitSpecialNode - Generate machine code for a target-independent node and

diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -179,12 +179,7 @@ TargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,
 
 void TargetLowering::AdjustInstrPostInstrSelection(MachineInstr *MI,
                                                    SDNode *Node) const {
-#ifndef NDEBUG
-  dbgs() << "If a target marks an instruction with "
-          "'hasPostISelHook', it must implement "
-          "TargetLowering::AdjustInstrPostInstrSelection!";
-#endif
-  llvm_unreachable(0);
+  // Do nothing unless the target overrides it.
 }
 
 //===----------------------------------------------------------------------===//

diff --git a/lib/Target/ARM/ARMISelLowering.cpp b/lib/Target/ARM/ARMISelLowering.cpp
@@ -5752,27 +5752,68 @@ ARMTargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,
   }
 }
 
+/// Generally, ARM instructions may be optionally encoded with a 's'
+/// bit. However, some opcodes have a compact encoding that forces an implicit
+/// 's' bit. List these exceptions here.
+static bool hasForcedCPSRDef(const MCInstrDesc &MCID) {
+  switch (MCID.getOpcode()) {
+  case ARM::t2ADDSri:
+  case ARM::t2ADDSrr:
+  case ARM::t2ADDSrs:
+  case ARM::t2SUBSri:
+  case ARM::t2SUBSrr:
+  case ARM::t2SUBSrs:
+    return true;
+  }
+  return false;
+}
+
 void ARMTargetLowering::AdjustInstrPostInstrSelection(MachineInstr *MI,
                                                       SDNode *Node) const {
-  // Adjust potentially 's' setting instructions after isel, i.e. ADC, SBC,
-  // RSB, RSC. Coming out of isel, they have an implicit CPSR def, but the
-  // optional operand is not filled in. If the carry bit is used, then change
-  // the optional operand to CPSR. Otherwise, remove the CPSR implicit def.
+  // Adjust potentially 's' setting instructions after isel, i.e. ADC, SBC, RSB,
+  // RSC. Coming out of isel, they have an implicit CPSR def, but the optional
+  // operand is still set to noreg. If needed, set the optional operand's
+  // register to CPSR, and remove the redundant implicit def.
+
   const MCInstrDesc &MCID = MI->getDesc();
-  if (Node->hasAnyUseOfValue(1)) {
-    MachineOperand &MO = MI->getOperand(MCID.getNumOperands() - 1);
-    MO.setReg(ARM::CPSR);
-    MO.setIsDef(true);
-  } else {
-    for (unsigned i = MCID.getNumOperands(), e = MI->getNumOperands();
-         i != e; ++i) {
-      const MachineOperand &MO = MI->getOperand(i);
-      if (MO.isReg() && MO.isDef() && MO.getReg() == ARM::CPSR) {
-        MI->RemoveOperand(i);
-        break;
-      }
+  unsigned ccOutIdx = MCID.getNumOperands() - 1;
+  bool forcedCPSR = hasForcedCPSRDef(MCID);
+
+  // Any ARM instruction that sets the 's' bit should specify an optional
+  // "cc_out" operand in the last operand position.
+  if (!MCID.hasOptionalDef() || !MCID.OpInfo[ccOutIdx].isOptionalDef()) {
+    assert(!forcedCPSR && "Optional cc_out operand required");
+    return;
+  }
+  // Look for an implicit def of CPSR added by MachineInstr ctor.
+  bool definesCPSR = false;
+  bool deadCPSR = false;
+  for (unsigned i = MCID.getNumOperands(), e = MI->getNumOperands();
+       i != e; ++i) {
+    const MachineOperand &MO = MI->getOperand(i);
+    if (MO.isReg() && MO.isDef() && MO.getReg() == ARM::CPSR) {
+      definesCPSR = true;
+      if (MO.isDead())
+        deadCPSR = true;
+      MI->RemoveOperand(i);
+      break;
     }
   }
+  if (!definesCPSR) {
+    assert(!forcedCPSR && "Optional cc_out operand required");
+    return;
+  }
+  assert(deadCPSR == !Node->hasAnyUseOfValue(1) && "inconsistent dead flag");
+
+  // If possible, select the encoding that does not set the 's' bit.
+  if (deadCPSR && !forcedCPSR)
+    return;
+
+  MachineOperand &MO = MI->getOperand(ccOutIdx);
+  MO.setReg(ARM::CPSR);
+  MO.setIsDef(true);
+  if (deadCPSR)
+    MO.setIsDead();
 }
 
 //===----------------------------------------------------------------------===//

diff --git a/lib/Target/ARM/ARMInstrInfo.td b/lib/Target/ARM/ARMInstrInfo.td
@@ -1026,7 +1026,7 @@ multiclass AsI1_rbin_irs<bits<4> opcod, string opc,
 }
 
 /// AsI1_rbin_s_is - Same as AsI1_rbin_s_is except it sets 's' bit by default.
-let hasPostISelHook = 1, isCodeGenOnly = 1, Defs = [CPSR] in {
+let isCodeGenOnly = 1, Defs = [CPSR] in {
 multiclass AsI1_rbin_s_is<bits<4> opcod, string opc,
                      InstrItinClass iii, InstrItinClass iir, InstrItinClass iis,
                         PatFrag opnode, bit Commutable = 0> {
@@ -1090,7 +1090,7 @@ multiclass AsI1_rbin_s_is<bits<4> opcod, string opc,
 }
 
 /// AsI1_bin_s_irs - Same as AsI1_bin_irs except it sets the 's' bit by default.
-let hasPostISelHook = 1, isCodeGenOnly = 1, Defs = [CPSR] in {
+let isCodeGenOnly = 1, Defs = [CPSR] in {
 multiclass AsI1_bin_s_irs<bits<4> opcod, string opc,
                      InstrItinClass iii, InstrItinClass iir, InstrItinClass iis,
                          PatFrag opnode, bit Commutable = 0> {
@@ -1278,7 +1278,7 @@ class AI_exta_rrot_np<bits<8> opcod, string opc>
 /// AI1_adde_sube_irs - Define instructions and patterns for adde and sube.
 multiclass AI1_adde_sube_irs<bits<4> opcod, string opc, PatFrag opnode,
                              string baseOpc, bit Commutable = 0> {
-  let hasPostISelHook = 1, Defs = [CPSR], Uses = [CPSR] in {
+  let Defs = [CPSR], Uses = [CPSR] in {
   def ri : AsI1<opcod, (outs GPR:$Rd), (ins GPR:$Rn, so_imm:$imm),
                 DPFrm, IIC_iALUi, opc, "\t$Rd, $Rn, $imm",
                [(set GPR:$Rd, CPSR, (opnode GPR:$Rn, so_imm:$imm, CPSR))]>,
@@ -1366,7 +1366,7 @@ multiclass AI1_adde_sube_irs<bits<4> opcod, string opc, PatFrag opnode,
 /// AI1_rsc_irs - Define instructions and patterns for rsc
 multiclass AI1_rsc_irs<bits<4> opcod, string opc, PatFrag opnode,
                        string baseOpc> {
-  let hasPostISelHook = 1, Defs = [CPSR], Uses = [CPSR] in {
+  let Defs = [CPSR], Uses = [CPSR] in {
   def ri : AsI1<opcod, (outs GPR:$Rd), (ins GPR:$Rn, so_imm:$imm),
                 DPFrm, IIC_iALUi, opc, "\t$Rd, $Rn, $imm",
                [(set GPR:$Rd, CPSR, (opnode so_imm:$imm, GPR:$Rn, CPSR))]>,

diff --git a/lib/Target/ARM/ARMInstrThumb2.td b/lib/Target/ARM/ARMInstrThumb2.td
@@ -592,7 +592,7 @@ multiclass T2I_rbin_irs<bits<4> opcod, string opc, PatFrag opnode> {
 
 /// T2I_bin_s_irs - Similar to T2I_bin_irs except it sets the 's' bit so the
 /// instruction modifies the CPSR register.
-let hasPostISelHook = 1, isCodeGenOnly = 1, Defs = [CPSR] in {
+let isCodeGenOnly = 1, Defs = [CPSR] in {
 multiclass T2I_bin_s_irs<bits<4> opcod, string opc,
                      InstrItinClass iii, InstrItinClass iir, InstrItinClass iis,
                          PatFrag opnode, bit Commutable = 0> {
@@ -738,7 +738,7 @@ multiclass T2I_adde_sube_irs<bits<4> opcod, string opc, PatFrag opnode,
 
 /// T2I_rbin_s_is - Same as T2I_rbin_irs except sets 's' bit and the register
 /// version is not needed since this is only for codegen.
-let hasPostISelHook = 1, isCodeGenOnly = 1, Defs = [CPSR] in {
+let isCodeGenOnly = 1, Defs = [CPSR] in {
 multiclass T2I_rbin_s_is<bits<4> opcod, string opc, PatFrag opnode> {
    // shifted imm
    def ri : T2sTwoRegImm<
@@ -1846,12 +1846,10 @@ defm t2SUBS : T2I_bin_s_irs <0b1101, "sub",
                              IIC_iALUi, IIC_iALUr, IIC_iALUsi,
                              BinOpFrag<(ARMsubc node:$LHS, node:$RHS)>>;
 
-let hasPostISelHook = 1 in {
 defm t2ADC  : T2I_adde_sube_irs<0b1010, "adc",
               BinOpWithFlagFrag<(ARMadde node:$LHS, node:$RHS, node:$FLAG)>, 1>;
 defm t2SBC  : T2I_adde_sube_irs<0b1011, "sbc",
               BinOpWithFlagFrag<(ARMsube node:$LHS, node:$RHS, node:$FLAG)>>;
-}
 
 // RSB
 defm t2RSB  : T2I_rbin_irs  <0b1110, "rsb",

diff --git a/test/CodeGen/ARM/2011-09-19-cpsr.ll b/test/CodeGen/ARM/2011-09-19-cpsr.ll
@@ -0,0 +1,54 @@
+; RUN: llc -march=thumb -mcpu=cortex-a8 < %s
+; rdar://problem/10137436: sqlite3 miscompile
+;
+; CHECK: subs
+; CHECK: cmp
+; CHECK: it
+
+target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:32:64-v128:32:128-a0:0:32-n32"
+target triple = "thumbv7-apple-ios4.0.0"
+
+declare i8* @__memset_chk(i8*, i32, i32, i32) nounwind
+
+define hidden fastcc i32 @sqlite3VdbeExec(i32* %p) nounwind {
+entry:
+  br label %sqlite3VarintLen.exit7424
+
+sqlite3VarintLen.exit7424:                        ; preds = %do.body.i7423
+  br label %do.body.i
+
+do.body.i:                                        ; preds = %do.body.i, %sqlite3VarintLen.exit7424
+  br i1 undef, label %do.body.i, label %sqlite3VarintLen.exit
+
+sqlite3VarintLen.exit:                            ; preds = %do.body.i
+  %sub2322 = add i64 undef, undef
+  br i1 undef, label %too_big, label %if.end2327
+
+if.end2327:                                       ; preds = %sqlite3VarintLen.exit
+  br i1 undef, label %if.end2341, label %no_mem
+
+if.end2341:                                       ; preds = %if.end2327
+  br label %for.body2355
+
+for.body2355:                                     ; preds = %for.body2355, %if.end2341
+  %add2366 = add nsw i32 undef, undef
+  br i1 undef, label %for.body2377, label %for.body2355
+
+for.body2377:                                     ; preds = %for.body2355
+  %conv23836154 = zext i32 %add2366 to i64
+  %sub2384 = sub i64 %sub2322, %conv23836154
+  %conv2385 = trunc i64 %sub2384 to i32
+  %len.0.i = select i1 undef, i32 %conv2385, i32 undef
+  %sub.i7384 = sub nsw i32 %len.0.i, 0
+  %call.i.i7385 = call i8* @__memset_chk(i8* undef, i32 0, i32 %sub.i7384, i32 undef) nounwind
+  unreachable
+
+too_big:                                          ; preds = %sqlite3VarintLen.exit
+  unreachable
+
+no_mem:                                           ; preds = %if.end2327, %for.body, %entry.no_mem_crit_edge
+  unreachable
+
+sqlite3ErrStr.exit:                               ; preds = %if.then82
+  unreachable
+}
diff --git a/utils/TableGen/CodeGenInstruction.cpp b/utils/TableGen/CodeGenInstruction.cpp
@@ -309,7 +309,6 @@ CodeGenInstruction::CodeGenInstruction(Record *R) : TheDef(R), Operands(R) {
   isReMaterializable = R->getValueAsBit("isReMaterializable");
   hasDelaySlot = R->getValueAsBit("hasDelaySlot");
   usesCustomInserter = R->getValueAsBit("usesCustomInserter");
-  hasPostISelHook = R->getValueAsBit("hasPostISelHook");
   hasCtrlDep   = R->getValueAsBit("hasCtrlDep");
   isNotDuplicable = R->getValueAsBit("isNotDuplicable");
   hasSideEffects = R->getValueAsBit("hasSideEffects");

diff --git a/utils/TableGen/CodeGenInstruction.h b/utils/TableGen/CodeGenInstruction.h
@@ -233,7 +233,6 @@ namespace llvm {
     bool isReMaterializable;
     bool hasDelaySlot;
     bool usesCustomInserter;
-    bool hasPostISelHook;
     bool hasCtrlDep;
     bool isNotDuplicable;
     bool hasSideEffects;

diff --git a/utils/TableGen/InstrInfoEmitter.cpp b/utils/TableGen/InstrInfoEmitter.cpp
@@ -288,7 +288,6 @@ void InstrInfoEmitter::emitRecord(const CodeGenInstruction &Inst, unsigned Num,
   if (Inst.isNotDuplicable)    OS << "|(1<<MCID::NotDuplicable)";
   if (Inst.Operands.hasOptionalDef) OS << "|(1<<MCID::HasOptionalDef)";
   if (Inst.usesCustomInserter) OS << "|(1<<MCID::UsesCustomInserter)";
-  if (Inst.hasPostISelHook)    OS << "|(1<<MCID::HasPostISelHook)";
   if (Inst.Operands.isVariadic)OS << "|(1<<MCID::Variadic)";
   if (Inst.hasSideEffects)     OS << "|(1<<MCID::UnmodeledSideEffects)";
   if (Inst.isAsCheapAsAMove)   OS << "|(1<<MCID::CheapAsAMove)";
@@ -345,7 +344,7 @@ void InstrInfoEmitter::emitEnums(raw_ostream &OS) {
 
   // We must emit the PHI opcode first...
   std::string Namespace = Target.getInstNamespace();
-  
+
   if (Namespace.empty()) {
     fprintf(stderr, "No instructions defined!\n");
     exit(1);