[llvm-commits] [llvm] r140134 - in /llvm/trunk: include/llvm/MC/MCInstrDesc.h lib/CodeGen/SelectionDAG/InstrEmitter.cpp lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/Target/ARM/ARMISelLowering.cpp lib/Target/ARM/ARMInstrInfo.td lib/Target/ARM/ARMInstrThumb2.td test/CodeGen/ARM/2011-09-19-cpsr.ll utils/TableGen/CodeGenInstruction.cpp utils/TableGen/CodeGenInstruction.h utils/TableGen/InstrInfoEmitter.cpp
Evan Cheng
evan.cheng at apple.com
Mon Sep 19 21:55:31 PDT 2011
Thanks! Just curious, could you have fixed the bug by adding hasPostIselHook = 1 to SUBS?
Evan
On Sep 19, 2011, at 8:17 PM, Andrew Trick <atrick at apple.com> wrote:
> Author: atrick
> Date: Mon Sep 19 22:17:40 2011
> New Revision: 140134
>
> URL: http://llvm.org/viewvc/llvm-project?rev=140134&view=rev
> Log:
> 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
>
> Added:
> llvm/trunk/test/CodeGen/ARM/2011-09-19-cpsr.ll
> Modified:
> llvm/trunk/include/llvm/MC/MCInstrDesc.h
> llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp
> llvm/trunk/lib/Target/ARM/ARMInstrInfo.td
> llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td
> llvm/trunk/utils/TableGen/CodeGenInstruction.cpp
> llvm/trunk/utils/TableGen/CodeGenInstruction.h
> llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp
>
> Modified: llvm/trunk/include/llvm/MC/MCInstrDesc.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCInstrDesc.h?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/MC/MCInstrDesc.h (original)
> +++ llvm/trunk/include/llvm/MC/MCInstrDesc.h Mon Sep 19 22:17:40 2011
> @@ -477,14 +477,6 @@
> 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
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp Mon Sep 19 22:17:40 2011
> @@ -763,8 +763,7 @@
> }
>
> // 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
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp Mon Sep 19 22:17:40 2011
> @@ -179,12 +179,7 @@
>
> 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.
> }
>
> //===----------------------------------------------------------------------===//
>
> Modified: llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp Mon Sep 19 22:17:40 2011
> @@ -5752,27 +5752,68 @@
> }
> }
>
> +/// 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();
> }
>
> //===----------------------------------------------------------------------===//
>
> Modified: llvm/trunk/lib/Target/ARM/ARMInstrInfo.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMInstrInfo.td?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMInstrInfo.td (original)
> +++ llvm/trunk/lib/Target/ARM/ARMInstrInfo.td Mon Sep 19 22:17:40 2011
> @@ -1026,7 +1026,7 @@
> }
>
> /// 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 @@
> }
>
> /// 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 @@
> /// 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 @@
> /// 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))]>,
>
> Modified: llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td (original)
> +++ llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td Mon Sep 19 22:17:40 2011
> @@ -592,7 +592,7 @@
>
> /// 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 @@
>
> /// 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 @@
> 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",
>
> Added: llvm/trunk/test/CodeGen/ARM/2011-09-19-cpsr.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/2011-09-19-cpsr.ll?rev=140134&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/2011-09-19-cpsr.ll (added)
> +++ llvm/trunk/test/CodeGen/ARM/2011-09-19-cpsr.ll Mon Sep 19 22:17:40 2011
> @@ -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
> +}
>
> Modified: llvm/trunk/utils/TableGen/CodeGenInstruction.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenInstruction.cpp?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/CodeGenInstruction.cpp (original)
> +++ llvm/trunk/utils/TableGen/CodeGenInstruction.cpp Mon Sep 19 22:17:40 2011
> @@ -309,7 +309,6 @@
> 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");
>
> Modified: llvm/trunk/utils/TableGen/CodeGenInstruction.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenInstruction.h?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/CodeGenInstruction.h (original)
> +++ llvm/trunk/utils/TableGen/CodeGenInstruction.h Mon Sep 19 22:17:40 2011
> @@ -233,7 +233,6 @@
> bool isReMaterializable;
> bool hasDelaySlot;
> bool usesCustomInserter;
> - bool hasPostISelHook;
> bool hasCtrlDep;
> bool isNotDuplicable;
> bool hasSideEffects;
>
> Modified: llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp?rev=140134&r1=140133&r2=140134&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/InstrInfoEmitter.cpp Mon Sep 19 22:17:40 2011
> @@ -288,7 +288,6 @@
> 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 @@
>
> // We must emit the PHI opcode first...
> std::string Namespace = Target.getInstNamespace();
> -
> +
> if (Namespace.empty()) {
> fprintf(stderr, "No instructions defined!\n");
> exit(1);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list