[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
Tue Sep 20 08:49:14 PDT 2011
On Sep 19, 2011, at 10:36 PM, Andrew Trick <atrick at apple.com> wrote:
> 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
I'm missing something obvious then. If the cpsr def isn't used, why can't codegen remove the implicit def and the optional def? The net effect is to change it to the non-S variant.
> 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.
I slightly disagree with this but this is something we can discuss offline.
Evan
>
> -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