Skip to content

Commit

Permalink
Revert r372285 "GlobalISel: Don't materialize immarg arguments to int…
Browse files Browse the repository at this point in the history
…rinsics"

This broke the Chromium build, causing it to fail with e.g.

  fatal error: error in backend: Cannot select: t362: v4i32 = X86ISD::VSHLI t392, Constant:i8<15>

See llvm-commits thread of r372285 for details.

This also reverts r372286, r372287, r372288, r372289, r372290, r372291,
r372292, r372293, r372296, and r372297, which seemed to depend on the
main commit.

> Encode them directly as an imm argument to G_INTRINSIC*.
>
> Since now intrinsics can now define what parameters are required to be
> immediates, avoid using registers for them. Intrinsics could
> potentially want a constant that isn't a legal register type. Also,
> since G_CONSTANT is subject to CSE and legalization, transforms could
> potentially obscure the value (and create extra work for the
> selector). The register bank of a G_CONSTANT is also meaningful, so
> this could throw off future folding and legalization logic for AMDGPU.
>
> This will be much more convenient to work with than needing to call
> getConstantVRegVal and checking if it may have failed for every
> constant intrinsic parameter. AMDGPU has quite a lot of intrinsics wth
> immarg operands, many of which need inspection during lowering. Having
> to find the value in a register is going to add a lot of boilerplate
> and waste compile time.
>
> SelectionDAG has always provided TargetConstant for constants which
> should not be legalized or materialized in a register. The distinction
> between Constant and TargetConstant was somewhat fuzzy, and there was
> no automatic way to force usage of TargetConstant for certain
> intrinsic parameters. They were both ultimately ConstantSDNode, and it
> was inconsistently used. It was quite easy to mis-select an
> instruction requiring an immediate. For SelectionDAG, start emitting
> TargetConstant for these arguments, and using timm to match them.
>
> Most of the work here is to cleanup target handling of constants. Some
> targets process intrinsics through intermediate custom nodes, which
> need to preserve TargetConstant usage to match the intrinsic
> expectation. Pattern inputs now need to distinguish whether a constant
> is merely compatible with an operand or whether it is mandatory.
>
> The GlobalISelEmitter needs to treat timm as a special case of a leaf
> node, simlar to MachineBasicBlock operands. This should also enable
> handling of patterns for some G_* instructions with immediates, like
> G_FENCE or G_EXTRACT.
>
> This does include a workaround for a crash in GlobalISelEmitter when
> ARM tries to uses "imm" in an output with a "timm" pattern source.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@372314 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
zmodem committed Sep 19, 2019
1 parent 4faf75d commit 0c2a34c
Show file tree
Hide file tree
Showing 79 changed files with 1,364 additions and 5,009 deletions.
5 changes: 0 additions & 5 deletions include/llvm/CodeGen/GlobalISel/InstructionSelector.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,6 @@ enum {
/// - OpIdx - Operand index
GIM_CheckIsMBB,

/// Check the specified operand is an Imm
/// - InsnID - Instruction ID
/// - OpIdx - Operand index
GIM_CheckIsImm,

/// Check if the specified operand is safe to fold into the current
/// instruction.
/// - InsnID - Instruction ID
Expand Down
14 changes: 1 addition & 13 deletions include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -690,19 +690,7 @@ bool InstructionSelector::executeMatchTable(
}
break;
}
case GIM_CheckIsImm: {
int64_t InsnID = MatchTable[CurrentIdx++];
int64_t OpIdx = MatchTable[CurrentIdx++];
DEBUG_WITH_TYPE(TgtInstructionSelector::getName(),
dbgs() << CurrentIdx << ": GIM_CheckIsImm(MIs[" << InsnID
<< "]->getOperand(" << OpIdx << "))\n");
assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
if (!State.MIs[InsnID]->getOperand(OpIdx).isImm()) {
if (handleReject() == RejectAndGiveUp)
return false;
}
break;
}

case GIM_CheckIsSafeToFold: {
int64_t InsnID = MatchTable[CurrentIdx++];
DEBUG_WITH_TYPE(TgtInstructionSelector::getName(),
Expand Down
3 changes: 0 additions & 3 deletions include/llvm/CodeGen/ScheduleDAGInstrs.h
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,6 @@ namespace llvm {
/// Returns a mask for which lanes get read/written by the given (register)
/// machine operand.
LaneBitmask getLaneMaskForMO(const MachineOperand &MO) const;

/// Returns true if the def register in \p MO has no uses.
bool deadDefHasNoUse(const MachineOperand &MO);
};

/// Creates a new SUnit and return a ptr to it.
Expand Down
5 changes: 0 additions & 5 deletions include/llvm/Target/TargetSelectionDAG.td
Original file line number Diff line number Diff line change
Expand Up @@ -848,11 +848,6 @@ class ImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm,
bit IsAPFloat = 0;
}

// Convenience wrapper for ImmLeaf to use timm/TargetConstant instead
// of imm/Constant.
class TImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm,
SDNode ImmNode = timm> : ImmLeaf<vt, pred, xform, ImmNode>;

// An ImmLeaf except that Imm is an APInt. This is useful when you need to
// zero-extend the immediate instead of sign-extend it.
//
Expand Down
27 changes: 6 additions & 21 deletions lib/CodeGen/GlobalISel/IRTranslator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1617,29 +1617,14 @@ bool IRTranslator::translateCall(const User &U, MachineIRBuilder &MIRBuilder) {
if (isa<FPMathOperator>(CI))
MIB->copyIRFlags(CI);

for (auto &Arg : enumerate(CI.arg_operands())) {
for (auto &Arg : CI.arg_operands()) {
// Some intrinsics take metadata parameters. Reject them.
if (isa<MetadataAsValue>(Arg.value()))
if (isa<MetadataAsValue>(Arg))
return false;

// If this is required to be an immediate, don't materialize it in a
// register.
if (CI.paramHasAttr(Arg.index(), Attribute::ImmArg)) {
if (ConstantInt *CI = dyn_cast<ConstantInt>(Arg.value())) {
// imm arguments are more convenient than cimm (and realistically
// probably sufficient), so use them.
assert(CI->getBitWidth() <= 64 &&
"large intrinsic immediates not handled");
MIB.addImm(CI->getSExtValue());
} else {
MIB.addFPImm(cast<ConstantFP>(Arg.value()));
}
} else {
ArrayRef<Register> VRegs = getOrCreateVRegs(*Arg.value());
if (VRegs.size() > 1)
return false;
MIB.addUse(VRegs[0]);
}
ArrayRef<Register> VRegs = getOrCreateVRegs(*Arg);
if (VRegs.size() > 1)
return false;
MIB.addUse(VRegs[0]);
}

// Add a MachineMemOperand if it is a target mem intrinsic.
Expand Down
10 changes: 2 additions & 8 deletions lib/CodeGen/ScheduleDAGInstrs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,6 @@ LaneBitmask ScheduleDAGInstrs::getLaneMaskForMO(const MachineOperand &MO) const
return TRI->getSubRegIndexLaneMask(SubReg);
}

bool ScheduleDAGInstrs::deadDefHasNoUse(const MachineOperand &MO) {
auto RegUse = CurrentVRegUses.find(MO.getReg());
if (RegUse == CurrentVRegUses.end())
return true;
return (RegUse->LaneMask & getLaneMaskForMO(MO)).none();
}

/// Adds register output and data dependencies from this SUnit to instructions
/// that occur later in the same scheduling region if they read from or write to
/// the virtual register defined at OperIdx.
Expand Down Expand Up @@ -409,7 +402,8 @@ void ScheduleDAGInstrs::addVRegDefDeps(SUnit *SU, unsigned OperIdx) {
}

if (MO.isDead()) {
assert(deadDefHasNoUse(MO) && "Dead defs should have no uses");
assert(CurrentVRegUses.find(Reg) == CurrentVRegUses.end() &&
"Dead defs should have no uses");
} else {
// Add data dependence to all uses we found so far.
const TargetSubtargetInfo &ST = MF.getSubtarget();
Expand Down
18 changes: 2 additions & 16 deletions lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4768,22 +4768,8 @@ void SelectionDAGBuilder::visitTargetIntrinsic(const CallInst &I,

// Add all operands of the call to the operand list.
for (unsigned i = 0, e = I.getNumArgOperands(); i != e; ++i) {
const Value *Arg = I.getArgOperand(i);
if (!I.paramHasAttr(i, Attribute::ImmArg)) {
Ops.push_back(getValue(Arg));
continue;
}

// Use TargetConstant instead of a regular constant for immarg.
EVT VT = TLI.getValueType(*DL, Arg->getType(), true);
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Arg)) {
assert(CI->getBitWidth() <= 64 &&
"large intrinsic immediates not handled");
Ops.push_back(DAG.getTargetConstant(*CI, SDLoc(), VT));
} else {
Ops.push_back(
DAG.getTargetConstantFP(*cast<ConstantFP>(Arg), SDLoc(), VT));
}
SDValue Op = getValue(I.getArgOperand(i));
Ops.push_back(Op);
}

SmallVector<EVT, 4> ValueVTs;
Expand Down
4 changes: 2 additions & 2 deletions lib/Target/AArch64/AArch64InstrFormats.td
Original file line number Diff line number Diff line change
Expand Up @@ -685,11 +685,11 @@ def logical_imm64_not : Operand<i64> {

// iXX_imm0_65535 predicates - True if the immediate is in the range [0,65535].
let ParserMatchClass = AsmImmRange<0, 65535>, PrintMethod = "printImmHex" in {
def i32_imm0_65535 : Operand<i32>, TImmLeaf<i32, [{
def i32_imm0_65535 : Operand<i32>, ImmLeaf<i32, [{
return ((uint32_t)Imm) < 65536;
}]>;

def i64_imm0_65535 : Operand<i64>, TImmLeaf<i64, [{
def i64_imm0_65535 : Operand<i64>, ImmLeaf<i64, [{
return ((uint64_t)Imm) < 65536;
}]>;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Target/AArch64/AArch64InstrInfo.td
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ def MOVbaseTLS : Pseudo<(outs GPR64:$dst), (ins),
let Uses = [ X9 ], Defs = [ X16, X17, LR, NZCV ] in {
def HWASAN_CHECK_MEMACCESS : Pseudo<
(outs), (ins GPR64noip:$ptr, i32imm:$accessinfo),
[(int_hwasan_check_memaccess X9, GPR64noip:$ptr, (i32 timm:$accessinfo))]>,
[(int_hwasan_check_memaccess X9, GPR64noip:$ptr, (i32 imm:$accessinfo))]>,
Sched<[]>;
}

Expand Down
Loading

0 comments on commit 0c2a34c

Please sign in to comment.