[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
Andrew Trick
atrick at apple.com
Mon Sep 19 22:36:57 PDT 2011
On Sep 19, 2011, at 9:55 PM, Evan Cheng wrote:
> Thanks! Just curious, could you have fixed the bug by adding hasPostIselHook = 1 to SUBS?
>
> Evan
>
t2SUBS was already under hasPostIselHook (T2I_bin_s_irs). Consequently, when its CPSR result was dead, the implicit def would be removed--that's wrong for any opcode that always sets the 's' bit. Hard-coding certain opcodes would be a sufficient fix, but I wanted some form of self-checking within the hook since these miscompiles are so nasty. With the self-checking in place, the hook flag serves no purpose but to avoid a vcall, which isn't very expensive at this point, but is another potential source of bugs when the .td file changes.
-Andy
> 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