[GlobalISel][AArch64] Always fall back on aarch64.neon.addp.*

[paquette]

Summary

If you have an intrinsic that doesn't have a 1:1 mapping with an instruction, and the types passed into that intrinsic aren't fully defined by their register banks, GISel can't actually map it to the right instruction today. As far as I know, addp is the only such intrinsic in AArch64.

Introduce a hacky workaround in the AArch64 legalizer to make sure we always fall back on this intrinsic. Goal here is to avoid impacting other backends as much as possible while we work towards a proper solution for the problem.

Also loosen some constraints on the legality of instructions with custom legalization, and factor out an assert. Before, we assumed that legalization would always *modify* an instruction *into* a legal state (or it'd be listed as legal based off the input LLTs). Since LLTs aren't sufficient here, this assumption falls over.

Also, we had the assumption that we'd be passing along LLTs into apply(), which isn't true here either.

https://bugs.llvm.org/show_bug.cgi?id=40968

[kristof.beyls]

lib/Target/AArch64/AArch64LegalizerInfo.cpp

518–527

Over time, more intrinsics get added.
So, making an exception based on a specific intrinsic name here seems like it's a time bomb waiting to go off when new intrinsics get added which have the same properties resulting in a silent codegen faults.
Wouldn't it be possible to check for the properties of the intrinsics that result in incorrect code generation and check on those properties rather than explicitly blacklist based on name?

[aemerson]

lib/Target/AArch64/AArch64LegalizerInfo.cpp

518–527

You're right, this is not a solution that fixes the real issue. It's a workaround that we plan to be in place until we settle on a way to fix this in the general case.

Unfortunately, barring disallowing all potentially int/fp type overloaded intrinsics during translation, we don't have a good way to detect these problematic ones automatically. This issue happens when no other attribute of the intrinsic call disambiguates the selected opcode, so element size etc isn't enough. Knowing that isn't generally possible unless we analyze all the selection patterns.

If you like, we could expand this fallback to something more general, up until falling back to all neon intrinsics. However I think that's probably unnecessary.

[paquette]

lib/Target/AArch64/AArch64LegalizerInfo.cpp

518–527

One way I can think of making this more general that might work is to check if both of the following hold:

1. The intrinsic is overloaded
2. There is at least one operand being passed into the intrinsic that is a vector/aggregate type

I think that both of these being true is sufficient to cover all such cases. Case (1) covers the fact that we have an intrinsic with more than one instruction associated with it in the first case.

Case (2) ought to work because for any scalar type, the register bank ought to cover for the missing type info. With vectors, the type info isn't implied by the register bank, and so that's where I think we're going wrong.

[arsenm]

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

81–84

Why is this necessary? This should hit the default in the switch. You could alternatively add an explicit case if you want to keep the comment

[arsenm]

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

448–453

This is really isLegalOrCustom. It may be useful to have both

lib/Target/AArch64/AArch64LegalizerInfo.h

38

Should this be added separately by D31359?

[paquette]

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

81–84

Because if you hand it a G_INTRINSIC, Q.Types ends up with a size of 0 (for some reason), so you crash.

lib/Target/AArch64/AArch64LegalizerInfo.h

38

I'm not sure what the status of that patch is, but if it's going to land, sure.

Otherwise, I guess we get everything through custom legalization anyway, so I'm not sure if it matters.

[aemerson]

lib/Target/AArch64/AArch64LegalizerInfo.h

38

I don't think we need that patch any more if we can do this with legalizeCustom, it's a subset of this functionality.

[aemerson]

LGTM, can you rename the test to something more specific like "fallback-ambiguous-addp-intrinsic.mir"

[paquette]

addressing minor comments before committing

• Changes to isLegal -> isLegalOrCustom
• Rename test

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

for (const auto &MMODescr : MMODescrs) {
     OS << MMODescr.SizeInBits << ", ";
   }
   OS << "}";
 
   return OS;
 }
 
 #ifndef NDEBUG
+// Make sure the rule won't (trivially) loop forever.
+static bool hasNoSimpleLoops(const LegalizeRule &Rule, const LegalityQuery &Q,
+                             const std::pair<unsigned, LLT> &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<unsigned, LLT> 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;
 
   switch (Rule.getAction()) {
   case FewerElements:
   case MoreElements: {
     if (!OldTy.isVector())
LegalizeActionStep LegalizeRuleSet::apply(const LegalityQuery &Query) const {
   for (const LegalizeRule &Rule : Rules) {
     if (Rule.match(Query)) {
       LLVM_DEBUG(dbgs() << ".. match\n");
       std::pair<unsigned, LLT> Mutation = Rule.determineMutation(Query);
       LLVM_DEBUG(dbgs() << ".. .. " << (unsigned)Rule.getAction() << ", "
                         << 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");
   }
   LLVM_DEBUG(dbgs() << ".. unsupported\n");
   return {LegalizeAction::Unsupported, 0, LLT{}};
 }
 
LegalizerInfo::getAction(const MachineInstr &MI,
   for (const auto &MMO : MI.memoperands())
     MemDescrs.push_back({8 * MMO->getSize() /* in bits */,
                          8 * MMO->getAlignment(),
                          MMO->getOrdering()});
 
   return getAction({MI.getOpcode(), Types, MemDescrs});
 }
 
 bool LegalizerInfo::isLegal(const MachineInstr &MI,
                             const MachineRegisterInfo &MRI) const {
-  return getAction(MI, MRI).Action == Legal;
+  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 {
   return false;
 }
 

lib/Target/AArch64/AArch64LegalizerInfo.h

public:
 
   bool legalizeCustom(MachineInstr &MI, MachineRegisterInfo &MRI,
                       MachineIRBuilder &MIRBuilder,
                       GISelChangeObserver &Observer) const override;
 
 private:
   bool legalizeVaArg(MachineInstr &MI, MachineRegisterInfo &MRI,
                      MachineIRBuilder &MIRBuilder) const;
+
+  bool legalizeIntrinsic(MachineInstr &MI, MachineRegisterInfo &MRI,
+                         MachineIRBuilder &MIRBuilder) const;
 };
 } // End llvm namespace.
 #endif

lib/Target/AArch64/AArch64LegalizerInfo.cpp

.fewerElementsIf(
       },
       [=](const LegalityQuery &Query) {
         LLT EltTy = Query.Types[0].getElementType();
         if (EltTy == s64)
           return std::make_pair(0, LLT::vector(2, 64));
         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)
       .widenScalarToNextPow2(0);
 
   getActionDefinitionsBuilder(G_BSWAP)
       .legalFor({s32, s64})
       .clampScalar(0, s16, s64)
bool AArch64LegalizerInfo::legalizeCustom(MachineInstr &MI,
                                           MachineIRBuilder &MIRBuilder,
                                           GISelChangeObserver &Observer) const {
   switch (MI.getOpcode()) {
   default:
     // No idea what to do.
     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 {
   MIRBuilder.setInstr(MI);
   MachineFunction &MF = MIRBuilder.getMF();
   unsigned Align = MI.getOperand(2).getImm();
   unsigned Dst = MI.getOperand(0).getReg();
   unsigned ListPtr = MI.getOperand(1).getReg();

test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir

 #
 # DEBUG-NEXT: G_BRCOND (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_BRINDIRECT (opcode {{[0-9]+}}): 1 type index
 # 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
 #
 # DEBUG-NEXT: G_ANYEXT (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_TRUNC (opcode {{[0-9]+}}): 2 type indices

test/CodeGen/AArch64/GlobalISel/select-addp.mir

+# 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
+
+...