From: Jessica Paquette Date: Mon, 11 Mar 2019 20:51:17 +0000 (+0000) Subject: [GlobalISel][AArch64] Always fall back on aarch64.neon.addp.* X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=cb9067728a598b76e123839c850ee5be3aec4fbb;p=llvm [GlobalISel][AArch64] Always fall back on aarch64.neon.addp.* Overloaded intrinsics aren't necessarily safe for instruction selection. One such intrinsic is aarch64.neon.addp.*. This is a temporary workaround to ensure that we always fall back on that intrinsic. Eventually this will be replaced with a proper solution. https://bugs.llvm.org/show_bug.cgi?id=40968 Differential Revision: https://reviews.llvm.org/D59062 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@355865 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h b/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h index cde9ff735af..b33f3f22c86 100644 --- a/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h +++ b/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h @@ -1055,6 +1055,8 @@ public: const MachineRegisterInfo &MRI) const; bool isLegal(const MachineInstr &MI, const MachineRegisterInfo &MRI) const; + bool isLegalOrCustom(const MachineInstr &MI, + const MachineRegisterInfo &MRI) const; virtual bool legalizeCustom(MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &MIRBuilder, diff --git a/lib/CodeGen/GlobalISel/LegalizerInfo.cpp b/lib/CodeGen/GlobalISel/LegalizerInfo.cpp index e17993987f0..32357ff657c 100644 --- a/lib/CodeGen/GlobalISel/LegalizerInfo.cpp +++ b/lib/CodeGen/GlobalISel/LegalizerInfo.cpp @@ -59,10 +59,30 @@ raw_ostream &LegalityQuery::print(raw_ostream &OS) const { } #ifndef NDEBUG +// Make sure the rule won't (trivially) loop forever. +static bool hasNoSimpleLoops(const LegalizeRule &Rule, const LegalityQuery &Q, + const std::pair &Mutation) { + switch (Rule.getAction()) { + case Custom: + case Lower: + case MoreElements: + case FewerElements: + break; + default: + return Q.Types[Mutation.first] != Mutation.second; + } + return true; +} + // Make sure the returned mutation makes sense for the match type. static bool mutationIsSane(const LegalizeRule &Rule, const LegalityQuery &Q, std::pair Mutation) { + // If the user wants a custom mutation, then we can't really say much about + // it. Return true, and trust that they're doing the right thing. + if (Rule.getAction() == Custom) + return true; + const unsigned TypeIdx = Mutation.first; const LLT OldTy = Q.Types[TypeIdx]; const LLT NewTy = Mutation.second; @@ -133,12 +153,7 @@ LegalizeActionStep LegalizeRuleSet::apply(const LegalityQuery &Query) const { << Mutation.first << ", " << Mutation.second << "\n"); assert(mutationIsSane(Rule, Query, Mutation) && "legality mutation invalid for match"); - - assert((Query.Types[Mutation.first] != Mutation.second || - Rule.getAction() == Lower || - Rule.getAction() == MoreElements || - Rule.getAction() == FewerElements) && - "Simple loop detected"); + assert(hasNoSimpleLoops(Rule, Query, Mutation) && "Simple loop detected"); return {Rule.getAction(), Mutation.first, Mutation.second}; } else LLVM_DEBUG(dbgs() << ".. no match\n"); @@ -435,6 +450,14 @@ bool LegalizerInfo::isLegal(const MachineInstr &MI, return getAction(MI, MRI).Action == Legal; } +bool LegalizerInfo::isLegalOrCustom(const MachineInstr &MI, + const MachineRegisterInfo &MRI) const { + auto Action = getAction(MI, MRI).Action; + // If the action is custom, it may not necessarily modify the instruction, + // so we have to assume it's legal. + return Action == Legal || Action == Custom; +} + bool LegalizerInfo::legalizeCustom(MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &MIRBuilder, GISelChangeObserver &Observer) const { @@ -644,7 +667,8 @@ const MachineInstr *llvm::machineFunctionIsIllegal(const MachineFunction &MF) { const MachineRegisterInfo &MRI = MF.getRegInfo(); for (const MachineBasicBlock &MBB : MF) for (const MachineInstr &MI : MBB) - if (isPreISelGenericOpcode(MI.getOpcode()) && !MLI->isLegal(MI, MRI)) + if (isPreISelGenericOpcode(MI.getOpcode()) && + !MLI->isLegalOrCustom(MI, MRI)) return &MI; } return nullptr; diff --git a/lib/Target/AArch64/AArch64LegalizerInfo.cpp b/lib/Target/AArch64/AArch64LegalizerInfo.cpp index 03933b1d098..c473dc490c5 100644 --- a/lib/Target/AArch64/AArch64LegalizerInfo.cpp +++ b/lib/Target/AArch64/AArch64LegalizerInfo.cpp @@ -64,6 +64,11 @@ AArch64LegalizerInfo::AArch64LegalizerInfo(const AArch64Subtarget &ST) { return std::make_pair(0, EltTy); }); + // HACK: Check that the intrinsic isn't ambiguous. + // (See: https://bugs.llvm.org/show_bug.cgi?id=40968) + getActionDefinitionsBuilder(G_INTRINSIC) + .custom(); + getActionDefinitionsBuilder(G_PHI) .legalFor({p0, s16, s32, s64}) .clampScalar(0, s16, s64) @@ -500,11 +505,30 @@ bool AArch64LegalizerInfo::legalizeCustom(MachineInstr &MI, return false; case TargetOpcode::G_VAARG: return legalizeVaArg(MI, MRI, MIRBuilder); + case TargetOpcode::G_INTRINSIC: + return legalizeIntrinsic(MI, MRI, MIRBuilder); } llvm_unreachable("expected switch to return"); } +bool AArch64LegalizerInfo::legalizeIntrinsic( + MachineInstr &MI, MachineRegisterInfo &MRI, + MachineIRBuilder &MIRBuilder) const { + // HACK: Don't allow faddp/addp for now. We don't pass down the type info + // necessary to get this right today. + // + // It looks like addp/faddp is the only intrinsic that's impacted by this. + // All other intrinsics fully describe the required types in their names. + // + // (See: https://bugs.llvm.org/show_bug.cgi?id=40968) + const MachineOperand &IntrinOp = MI.getOperand(1); + if (IntrinOp.isIntrinsicID() && + IntrinOp.getIntrinsicID() == Intrinsic::aarch64_neon_addp) + return false; + return true; +} + bool AArch64LegalizerInfo::legalizeVaArg(MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &MIRBuilder) const { diff --git a/lib/Target/AArch64/AArch64LegalizerInfo.h b/lib/Target/AArch64/AArch64LegalizerInfo.h index c5979d8bbe5..9472a4e9c93 100644 --- a/lib/Target/AArch64/AArch64LegalizerInfo.h +++ b/lib/Target/AArch64/AArch64LegalizerInfo.h @@ -34,6 +34,9 @@ public: private: bool legalizeVaArg(MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &MIRBuilder) const; + + bool legalizeIntrinsic(MachineInstr &MI, MachineRegisterInfo &MRI, + MachineIRBuilder &MIRBuilder) const; }; } // End llvm namespace. #endif diff --git a/test/CodeGen/AArch64/GlobalISel/fallback-ambiguous-addp-intrinsic.mir b/test/CodeGen/AArch64/GlobalISel/fallback-ambiguous-addp-intrinsic.mir new file mode 100644 index 00000000000..e0a7f71ce55 --- /dev/null +++ b/test/CodeGen/AArch64/GlobalISel/fallback-ambiguous-addp-intrinsic.mir @@ -0,0 +1,32 @@ +# RUN: llc -mtriple aarch64-unknown-unknown -O0 -start-before=legalizer -pass-remarks-missed=gisel* %s -o - 2>&1 | FileCheck %s +# +# Check that we fall back on @llvm.aarch64.neon.addp and ensure that we get the +# correct instruction. +# https://bugs.llvm.org/show_bug.cgi?id=40968 + +--- | + define <2 x float> @foo(<2 x float> %v1, <2 x float> %v2) { + entry: + %v3 = call <2 x float> @llvm.aarch64.neon.addp.v2f32(<2 x float> %v1, <2 x float> %v2) + ret <2 x float> %v3 + } + declare <2 x float> @llvm.aarch64.neon.addp.v2f32(<2 x float>, <2 x float>) +... +--- +name: foo +alignment: 2 +tracksRegLiveness: true +body: | + bb.1.entry: + liveins: $d0, $d1 + ; CHECK: remark: + ; CHECK-SAME: unable to legalize instruction: %2:_(<2 x s32>) = G_INTRINSIC intrinsic(@llvm.aarch64.neon.addp), %0:_(<2 x s32>), %1:_(<2 x s32>) + ; CHECK: faddp + ; CHECK-NOT: addp + %0:_(<2 x s32>) = COPY $d0 + %1:_(<2 x s32>) = COPY $d1 + %2:_(<2 x s32>) = G_INTRINSIC intrinsic(@llvm.aarch64.neon.addp), %0(<2 x s32>), %1(<2 x s32>) + $d0 = COPY %2(<2 x s32>) + RET_ReallyLR implicit $d0 + +... diff --git a/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir b/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir index 5a0e8d800c5..8b6214b5cad 100644 --- a/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir +++ b/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir @@ -151,7 +151,7 @@ # DEBUG: .. the first uncovered type index: 1, OK # # DEBUG-NEXT: G_INTRINSIC (opcode {{[0-9]+}}): 0 type indices -# DEBUG: .. type index coverage check SKIPPED: no rules defined +# DEBUG: .. type index coverage check SKIPPED: user-defined predicate detected # # DEBUG-NEXT: G_INTRINSIC_W_SIDE_EFFECTS (opcode {{[0-9]+}}): 0 type indices # DEBUG: .. type index coverage check SKIPPED: no rules defined