[aarch64] Support APInt and APFloat in ImmLeaf subclasses and make AArch64 use them.

[dsanders]

Summary

The purpose of this patch is to expose more information about ImmLeaf-like
PatLeaf's so that GlobalISel can learn to import them. Previously, ImmLeaf
could only be used to test int64_t's produced by sign-extending an APInt.
Other tests on immediates had to use the generic PatLeaf and extract the
constant using C++.

With this patch, tablegen will know how to generate predicates for APInt,
and APFloat. This will allow it to 'do the right thing' for both SelectionDAG
and GlobalISel which require different methods of extracting the immediate
from the IR.

This is NFC for SelectionDAG since the new code is equivalent to the
previous code. It's also NFC for FastISel because FastIselShouldIgnore is 1
for the ImmLeaf subclasses. Enabling FastIselShouldIgnore == 0 for these new
subclasses will require a significant re-factor of FastISel.

For GlobalISel, it's currently NFC because the relevant code to import the
affected rules is not yet present. This will be added in a later patch.

Depends on D36086

[dsanders]

Removed the reference to zero-extending APInt's from the summary since this isn't in the patch anymore. IntImmLeaf with a predicate that uses Imm->getZExtValue() is equivalent so I didn't see a need to make uint64_t predicates separately.

[dsanders]

ping

[dsanders]

Refresh

[rovka]

I can see why you need to change the patterns to be more general, but is the distinction between APInt, APFloat and 64-bit values really relevant? I looked a bit at the following patch which uses this for GlobalISel and I'm wondering if we could just get away with checking for each immediate a combination of isCImm, isFPImm and getBitWidth (so we wouldn't need to introduce different states for different kinds of immediates).

I'm sure I'm missing some details, so what's the catch? :)

utils/TableGen/CodeGenDAGPatterns.cpp

879

Redundant else.

utils/TableGen/GlobalISelEmitter.cpp

2572

Shouldn't this be +1?

2585

Could you get rid of this repetition? If we don't expect many of these, we could probably use a copy_if into another container, but if you think that might affect performance too much you can probably just extract a ForEachFilteredImm lambda/helper. I can think of other variations but I'll let you choose whatever you think is best.

[dsanders]

Sorry for the slow reply, I was on holiday last week.

In D36534#856660, @rovka wrote:

I can see why you need to change the patterns to be more general, but is the distinction between APInt, APFloat and 64-bit values really relevant? I looked a bit at the following patch which uses this for GlobalISel and I'm wondering if we could just get away with checking for each immediate a combination of isCImm, isFPImm and getBitWidth (so we wouldn't need to introduce different states for different kinds of immediates).

I'm sure I'm missing some details, so what's the catch? :)

We can potentially remove int64_t later (see below) but we need to keep the distinction between APInt and APFloat. In SelectionDAG, the APInt kind corresponds to ConstantSDNode::getAPIntValue() while the APFloat kind corresponds to ConstantFPSDNode::getValueAPF(). SelectionDAG needs to know which to use when emitting the predicate code. We could generate code to figure it out but we'd be doing work at run-time that we can easily do at compile-time. Similarly, GlobalISel needs to know whether to use ConstantInt::getValue() or ConstantFP::getValueAPF() and knows which one during tablegen so it makes sense to do this at compile-time.

The reason I think we can remove int64_t is that it's equivalent to an APInt predicate where Imm is always accessed with Imm->getSExtValue(). I haven't done this because it requires a change to every target.

utils/TableGen/GlobalISelEmitter.cpp

2572

Yes, it should. I've already fixed a similar issue upstream so it should be fixed in my next rebase.

2585

Ok

[dsanders]

Rebase and fix nits

[javed.absar]

include/llvm/Target/TargetSelectionDAG.td

685

I am no expert on this, but would the comment be better as 'Is the data type... "
The 'Should the data... " seems to suggest an uncertainty (not sure), which is not the case here.

[dsanders]

include/llvm/Target/TargetSelectionDAG.td

685

Both make sense to me since in both cases tablegen is asking the question of the user and the user supplies the answer in the field. If there's a preference for a particular phrasing then I'm happy to change it.

[bjope]

include/llvm/Target/TargetSelectionDAG.td

697

The comment above says that this the same as ImmLeaf except that Imm is an APInt. But you have also changed the default setting for FastIselShouldIgnore. So if that matters for some target you can't just replace ImmLeaf by IntImmLeaf.

I actually do not get the comments in the summary about the change being NFC for FastISel due to this. You changed lots of AArch64 definitions to use IntImmLeaf/FPImmLeaf instead of using PatLeaf. But I guess we could see PatLeaf as having FastISelShouldIgnore=0 (because there is no ignore in FastISel for PatLeaf, right?). So to me it looks like you have made changes to how FastISel will handle all those definitions that you have updated to use IntImmLeaf/FPImmLeaf.

I don't know much about FastISel so maybe I misunderstand something. And maybe AArch64 doesn't ever use FastISel. If that is the case then your patch could be seen as NFC, but then I still don't get the point in why it was important to set FastIselShouldIgnore = 1 here.

[dsanders]

include/llvm/Target/TargetSelectionDAG.td

697

The comment above says that this the same as ImmLeaf except that Imm is an APInt. But you have
also changed the default setting for FastIselShouldIgnore. So if that matters for some target you can't
just replace ImmLeaf by IntImmLeaf.

For SelectionDAG and GlobalISel an IntImmLeaf is the same as an ImmLeaf except that the Imm variable is an APInt. This would also be the case for FastISel but FastISel doesn't know how to generate predicates that use APInt at the moment.

I actually do not get the comments in the summary about the change being NFC for FastISel due to
this. You changed lots of AArch64 definitions to use IntImmLeaf/FPImmLeaf instead of using PatLeaf. > But I guess we could see PatLeaf as having FastISelShouldIgnore=0 (because there is no ignore in
FastISel for PatLeaf, right?). So to me it looks like you have made changes to how FastISel will handle
all those definitions that you have updated to use IntImmLeaf/FPImmLeaf.

I don't know much about FastISel so maybe I misunderstand something. And maybe AArch64 doesn't
ever use FastISel. If that is the case then your patch could be seen as NFC, but then I still don't get the
point in why it was important to set FastIselShouldIgnore = 1 here.

FastISelEmitter.cpp doesn't have any code to import patterns from PatFrag/PatLeaf so they are effectively treated as FastIselShouldIgnore=1. The main issue preventing them from being imported into FastISel seems to be the same one that affects GlobalISel which is that the C++ in the predicates is written to use SDNode objects but those don't exist in FastISel/GlobalISel.

In the long term, FastISel could be extended to support APInt/APFloat predicates at which point we can drop the FastIselShouldIgnore=1.

[bjope]

include/llvm/Target/TargetSelectionDAG.td

697

FastISelEmitter.cpp doesn't have any code to import patterns from PatFrag/PatLeaf so they are effectively treated as FastIselShouldIgnore=1.

Ok I see. That explains why this is NFC also for FastISel. Because you have replaced a couple of PatLeaf by IntImmLeaf in this patch.

But I guess that FastISelEmitter.cpp has support for importing ImmLeaf patterns. So it is still confusing to say that IntImmLeaf is the same as an ImmLeaf except the Imm being an APInt. It is also different when it comes to FastIselShouldIgnore setting. So replacing ImmLeaf by IntImmLeaf would not be an NFC change. Maybe not a big deal, but that code comment fooled me for awhile until.

[dsanders]

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

[dsanders]

'Should the ... be ...?' comments changed to 'Is the ... a ...?'
Clarified that IntImmLeaf is not a replacement for ImmLeaf but rather a
replacement for PatLeaf with immediate predicates that ImmLeaf can't handle.

[aemerson]

In D36534#893372, @dsanders wrote:

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

I'm just curious, what's the issue with it returning a StringRef?

[dsanders]

In D36534#893457, @aemerson wrote:

In D36534#893372, @dsanders wrote:

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

I'm just curious, what's the issue with it returning a StringRef?

getPredCode() needs to build the code from several substrings depending on the data type of the ImmLeaf subclass. StringRef's don't own the data they reference so it will end up referencing a local that's destroyed by the exit from getPredCode().

[qcolombet]

LGTM

include/llvm/ADT/APFloat.h

if (&getSemantics() != &RHS.getSemantics())
       return false;
     if (usesLayout<IEEEFloat>(getSemantics()))
       return U.IEEE.bitwiseIsEqual(RHS.U.IEEE);
     if (usesLayout<DoubleAPFloat>(getSemantics()))
       return U.Double.bitwiseIsEqual(RHS.U.Double);
     llvm_unreachable("Unexpected semantics");
   }
 
+  /// We don't rely on operator== working on double values, as
+  /// it returns true for things that are clearly not equal, like -0.0 and 0.0.
+  /// As such, this method can be used to do an exact bit-for-bit comparison of
+  /// two floating point values.
+  ///
+  /// We leave the version with the double argument here because it's just so
+  /// convenient to write "2.0" and the like.  Without this function we'd
+  /// have to duplicate its logic everywhere it's called.
+  bool isExactlyValue(double V) const {
+    bool ignored;
+    APFloat Tmp(V);
+    Tmp.convert(getSemantics(), APFloat::rmNearestTiesToEven, &ignored);
+    return bitwiseIsEqual(Tmp);
+  }
+
   unsigned int convertToHexString(char *DST, unsigned int HexDigits,
                                   bool UpperCase, roundingMode RM) const {
     APFLOAT_DISPATCH_ON_SEMANTICS(
         convertToHexString(DST, HexDigits, UpperCase, RM));
   }
 
   bool isZero() const { return getCategory() == fcZero; }
   bool isInfinity() const { return getCategory() == fcInfinity; }

include/llvm/CodeGen/SelectionDAGNodes.h

public:
   /// it returns true for things that are clearly not equal, like -0.0 and 0.0.
   /// As such, this method can be used to do an exact bit-for-bit comparison of
   /// two floating point values.
 
   /// We leave the version with the double argument here because it's just so
   /// convenient to write "2.0" and the like.  Without this function we'd
   /// have to duplicate its logic everywhere it's called.
   bool isExactlyValue(double V) const {
-    bool ignored;
-    APFloat Tmp(V);
-    Tmp.convert(Value->getValueAPF().getSemantics(),
-                APFloat::rmNearestTiesToEven, &ignored);
-    return isExactlyValue(Tmp);
+    return Value->getValueAPF().isExactlyValue(V);
   }
   bool isExactlyValue(const APFloat& V) const;
 
   static bool isValueValidForType(EVT VT, const APFloat& Val);
 
   static bool classof(const SDNode *N) {
     return N->getOpcode() == ISD::ConstantFP ||
            N->getOpcode() == ISD::TargetConstantFP;

include/llvm/Target/TargetSelectionDAG.td

 //
 // this is a more convenient form to match 'imm' nodes in than PatLeaf and also
 // is preferred over using PatLeaf because it allows the code generator to
 // reason more about the constraint.
 //
 // If FastIsel should ignore all instructions that have an operand of this type,
 // the FastIselShouldIgnore flag can be set.  This is an optimization to reduce
 // the code size of the generated fast instruction selector.
-class ImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm>
-  : PatFrag<(ops), (vt imm), [{}], xform> {
+class ImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm,
+              SDNode ImmNode = imm>
+  : PatFrag<(ops), (vt ImmNode), [{}], xform> {
   let ImmediateCode = pred;
   bit FastIselShouldIgnore = 0;
+
+  // Is the data type of the immediate an APInt?
+  bit IsAPInt = 0;
+
+  // Is the data type of the immediate an APFloat?
+  bit IsAPFloat = 0;
 }
 
+// An ImmLeaf except that Imm is an APInt. This is useful when you need to
+// zero-extend the immediate instead of sign-extend it.
+//
+// Note that FastISel does not currently understand IntImmLeaf and will not
+// generate code for rules that make use of it. As such, it does not make sense
+// to replace ImmLeaf with IntImmLeaf. However, replacing PatLeaf with an
+// IntImmLeaf will allow GlobalISel to import the rule.
+class IntImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm>
+    : ImmLeaf<vt, pred, xform> {
+  let IsAPInt = 1;
+  let FastIselShouldIgnore = 1;
+}
+
+// An ImmLeaf except that Imm is an APFloat.
+//
+// Note that FastISel does not currently understand FPImmLeaf and will not
+// generate code for rules that make use of it.
+class FPImmLeaf<ValueType vt, code pred, SDNodeXForm xform = NOOP_SDNodeXForm>
+  : ImmLeaf<vt, pred, xform, fpimm> {
+  let IsAPFloat = 1;
+  let FastIselShouldIgnore = 1;
+}
 
 // Leaf fragments.
 
 def vtInt      : PatLeaf<(vt),  [{ return N->getVT().isInteger(); }]>;
 def vtFP       : PatLeaf<(vt),  [{ return N->getVT().isFloatingPoint(); }]>;
 
 def immAllOnesV: PatLeaf<(build_vector), [{
   return ISD::isBuildVectorAllOnes(N);

lib/Target/AArch64/AArch64InstrFormats.td

let DiagnosticType = "LogicalSecondSource" in {
   }
   def LogicalImm32NotOperand : AsmOperandClass {
     let Name = "LogicalImm32Not";
   }
   def LogicalImm64NotOperand : AsmOperandClass {
     let Name = "LogicalImm64Not";
   }
 }
-def logical_imm32 : Operand<i32>, PatLeaf<(imm), [{
-  return AArch64_AM::isLogicalImmediate(N->getZExtValue(), 32);
+def logical_imm32 : Operand<i32>, IntImmLeaf<i32, [{
+  return AArch64_AM::isLogicalImmediate(Imm.getZExtValue(), 32);
 }], logical_imm32_XFORM> {
   let PrintMethod = "printLogicalImm32";
   let ParserMatchClass = LogicalImm32Operand;
 }
-def logical_imm64 : Operand<i64>, PatLeaf<(imm), [{
-  return AArch64_AM::isLogicalImmediate(N->getZExtValue(), 64);
+def logical_imm64 : Operand<i64>, IntImmLeaf<i64, [{
+  return AArch64_AM::isLogicalImmediate(Imm.getZExtValue(), 64);
 }], logical_imm64_XFORM> {
   let PrintMethod = "printLogicalImm64";
   let ParserMatchClass = LogicalImm64Operand;
 }
 def logical_imm32_not : Operand<i32> {
   let ParserMatchClass = LogicalImm32NotOperand;
 }
 def logical_imm64_not : Operand<i64> {

 class arith_extended_reg32to64<ValueType Ty> : Operand<Ty>,
                     ComplexPattern<Ty, 2, "SelectArithExtendedRegister", []> {
   let PrintMethod = "printExtendedRegister";
   let MIOperandInfo = (ops GPR32, arith_extend64);
 }
 
 // Floating-point immediate.
 def fpimm16 : Operand<f16>,
-              PatLeaf<(f16 fpimm), [{
-      return AArch64_AM::getFP16Imm(N->getValueAPF()) != -1;
+              FPImmLeaf<f16, [{
+      return AArch64_AM::getFP16Imm(Imm) != -1;
     }], SDNodeXForm<fpimm, [{
       APFloat InVal = N->getValueAPF();
       uint32_t enc = AArch64_AM::getFP16Imm(InVal);
       return CurDAG->getTargetConstant(enc, SDLoc(N), MVT::i32);
     }]>> {
   let ParserMatchClass = FPImmOperand;
   let PrintMethod = "printFPImmOperand";
 }
 def fpimm32 : Operand<f32>,
-              PatLeaf<(f32 fpimm), [{
-      return AArch64_AM::getFP32Imm(N->getValueAPF()) != -1;
+              FPImmLeaf<f32, [{
+      return AArch64_AM::getFP32Imm(Imm) != -1;
     }], SDNodeXForm<fpimm, [{
       APFloat InVal = N->getValueAPF();
       uint32_t enc = AArch64_AM::getFP32Imm(InVal);
       return CurDAG->getTargetConstant(enc, SDLoc(N), MVT::i32);
     }]>> {
   let ParserMatchClass = FPImmOperand;
   let PrintMethod = "printFPImmOperand";
 }
 def fpimm64 : Operand<f64>,
-              PatLeaf<(f64 fpimm), [{
-      return AArch64_AM::getFP64Imm(N->getValueAPF()) != -1;
+              FPImmLeaf<f64, [{
+      return AArch64_AM::getFP64Imm(Imm) != -1;
     }], SDNodeXForm<fpimm, [{
       APFloat InVal = N->getValueAPF();
       uint32_t enc = AArch64_AM::getFP64Imm(InVal);
       return CurDAG->getTargetConstant(enc, SDLoc(N), MVT::i32);
     }]>> {
   let ParserMatchClass = FPImmOperand;
   let PrintMethod = "printFPImmOperand";
 }
 
 def fpimm8 : Operand<i32> {
   let ParserMatchClass = FPImmOperand;
   let PrintMethod = "printFPImmOperand";
 }
 
-def fpimm0 : PatLeaf<(fpimm), [{
-  return N->isExactlyValue(+0.0);
+def fpimm0 : FPImmLeaf<fAny, [{
+  return Imm.isExactlyValue(+0.0);
 }]>;
 
 // Vector lane operands
 class AsmVectorIndex<string Suffix> : AsmOperandClass {
   let Name = "VectorIndex" # Suffix;
   let DiagnosticType = "InvalidIndex" # Suffix;
 }
 def VectorIndex1Operand : AsmVectorIndex<"1">;
def VectorIndexD : Operand<i64>, ImmLeaf<i64, [{
   let PrintMethod = "printVectorIndex";
   let MIOperandInfo = (ops i64imm);
 }
 
 // 8-bit immediate for AdvSIMD where 64-bit values of the form:
 // aaaaaaaa bbbbbbbb cccccccc dddddddd eeeeeeee ffffffff gggggggg hhhhhhhh
 // are encoded as the eight bit value 'abcdefgh'.
 def simdimmtype10 : Operand<i32>,
-                    PatLeaf<(f64 fpimm), [{
-      return AArch64_AM::isAdvSIMDModImmType10(N->getValueAPF()
-                                               .bitcastToAPInt()
-                                               .getZExtValue());
+                    FPImmLeaf<f64, [{
+      return AArch64_AM::isAdvSIMDModImmType10(
+                 Imm.bitcastToAPInt().getZExtValue());
     }], SDNodeXForm<fpimm, [{
       APFloat InVal = N->getValueAPF();
       uint32_t enc = AArch64_AM::encodeAdvSIMDModImmType10(N->getValueAPF()
                                                            .bitcastToAPInt()
                                                            .getZExtValue());
       return CurDAG->getTargetConstant(enc, SDLoc(N), MVT::i32);
     }]>> {
   let ParserMatchClass = SIMDImmType10Operand;

utils/TableGen/CodeGenDAGPatterns.h

public:
   std::string getFnName() const;
 
   /// getCodeToRunOnSDNode - Return the code for the function body that
   /// evaluates this predicate.  The argument is expected to be in "Node",
   /// not N.  This handles casting and conversion to a concrete node type as
   /// appropriate.
   std::string getCodeToRunOnSDNode() const;
 
+  /// Get the data type of the argument to getImmediatePredicateCode().
+  std::string getImmType() const;
+
 private:
   std::string getPredCode() const;
   std::string getImmCode() const;
+  bool immCodeUsesAPInt() const;
+  bool immCodeUsesAPFloat() const;
 };
 
 
 /// FIXME: TreePatternNode's can be shared in some cases (due to dag-shaped
 /// patterns), and as such should be ref counted.  We currently just leak all
 /// TreePatternNode objects!
 class TreePatternNode {
   /// The type of each node result.  Before and during type inference, each

utils/TableGen/CodeGenDAGPatterns.cpp

 std::string TreePredicateFn::getPredCode() const {
   return PatFragRec->getRecord()->getValueAsString("PredicateCode");
 }
 
 std::string TreePredicateFn::getImmCode() const {
   return PatFragRec->getRecord()->getValueAsString("ImmediateCode");
 }
 
+bool TreePredicateFn::immCodeUsesAPInt() const {
+  return getOrigPatFragRecord()->getRecord()->getValueAsBit("IsAPInt");
+}
+
+bool TreePredicateFn::immCodeUsesAPFloat() const {
+  bool Unset;
+  // The return value will be false when IsAPFloat is unset.
+  return getOrigPatFragRecord()->getRecord()->getValueAsBitOrUnset("IsAPFloat",
+                                                                   Unset);
+}
+
+std::string TreePredicateFn::getImmType() const {
+  if (immCodeUsesAPInt())
+    return "const APInt &";
+  if (immCodeUsesAPFloat())
+    return "const APFloat &";
+  return "int64_t";
+}
 
 /// isAlwaysTrue - Return true if this is a noop predicate.
 bool TreePredicateFn::isAlwaysTrue() const {
   return getPredCode().empty() && getImmCode().empty();
 }
 
 /// Return the name to use in the generated code to reference this, this is
 /// "Predicate_foo" if from a pattern fragment "foo".
 std::string TreePredicateFn::getFnName() const {
   return "Predicate_" + PatFragRec->getRecord()->getName().str();
 }
 
 /// getCodeToRunOnSDNode - Return the code for the function body that
 /// evaluates this predicate.  The argument is expected to be in "Node",
 /// not N.  This handles casting and conversion to a concrete node type as
 /// appropriate.
 std::string TreePredicateFn::getCodeToRunOnSDNode() const {
   // Handle immediate predicates first.
   std::string ImmCode = getImmCode();
   if (!ImmCode.empty()) {
-    std::string Result =
-      "    int64_t Imm = cast<ConstantSDNode>(Node)->getSExtValue();\n";
+    std::string Result = "    " + getImmType() + " Imm = ";
+    if (immCodeUsesAPFloat())
+      Result += "cast<ConstantFPSDNode>(Node)->getValueAPF();\n";
+    else if (immCodeUsesAPInt())
+      Result += "cast<ConstantSDNode>(Node)->getAPIntValue();\n";
+    else
+      Result += "cast<ConstantSDNode>(Node)->getSExtValue();\n";
     return Result + ImmCode;
   }
 
   // Handle arbitrary node predicates.
   assert(!getPredCode().empty() && "Don't have any predicate code!");
   std::string ClassName;
   if (PatFragRec->getOnlyTree()->isLeaf())
     ClassName = "SDNode";

utils/TableGen/GlobalISelEmitter.cpp

Error importExplicitUseRenderer(BuildMIAction &DstMIBuilder,
                                   TreePatternNode *DstChild,
                                   const InstructionMatcher &InsnMatcher) const;
   Error importDefaultOperandRenderers(BuildMIAction &DstMIBuilder,
                                       DagInit *DefaultOps) const;
   Error
   importImplicitDefRenderers(BuildMIAction &DstMIBuilder,
                              const std::vector<Record *> &ImplicitDefs) const;
 
+  void emitImmPredicates(raw_ostream &OS,
+                         std::function<bool(const Record *R)> Filter);
+
   /// Analyze pattern \p P, returning a matcher for it if possible.
   /// Otherwise, return an Error explaining why we don't support it.
   Expected<RuleMatcher> runOnPattern(const PatternToMatch &P);
 
   void declareSubtargetFeature(Record *Predicate);
 };
 
 void GlobalISelEmitter::gatherNodeEquivs() {
Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
 
   M.addAction<ConstrainOperandsToDefinitionAction>(0);
 
   // We're done with this pattern!  It's eligible for GISel emission; return it.
   ++NumPatternImported;
   return std::move(M);
 }
 
+// Emit imm predicate table and an enum to reference them with.
+// The 'Predicate_' part of the name is redundant but eliminating it is more
+// trouble than it's worth.
+void GlobalISelEmitter::emitImmPredicates(
+    raw_ostream &OS, std::function<bool(const Record *R)> Filter) {
+  std::vector<const Record *> MatchedRecords;
+  const auto &Defs = RK.getAllDerivedDefinitions("PatFrag");
+  std::copy_if(Defs.begin(), Defs.end(), std::back_inserter(MatchedRecords),
+               [&](Record *Record) {
+                 return !Record->getValueAsString("ImmediateCode").empty() &&
+                        Filter(Record);
+               });
+
+  OS << "// PatFrag predicates.\n"
+     << "enum {\n";
+  StringRef EnumeratorSeparator = " = GIPFP_Invalid + 1,\n";
+  for (const auto *Record : MatchedRecords) {
+    OS << "  GIPFP_Predicate_" << Record->getName() << EnumeratorSeparator;
+    EnumeratorSeparator = ",\n";
+  }
+  OS << "};\n";
+  for (const auto *Record : MatchedRecords)
+    OS << "  static bool Predicate_" << Record->getName() << "(int64_t Imm) {"
+       << Record->getValueAsString("ImmediateCode") << "  }\n";
+  OS << "static InstructionSelector::ImmediatePredicateFn ImmPredicateFns[] = "
+        "{\n"
+     << "  nullptr,\n";
+  for (const auto *Record : MatchedRecords)
+    OS << "  Predicate_" << Record->getName() << ",\n";
+  OS << "};\n";
+}
+
 void GlobalISelEmitter::run(raw_ostream &OS) {
   // Track the GINodeEquiv definitions.
   gatherNodeEquivs();
 
   emitSourceFileHeader(("Global Instruction Selector for the " +
                        Target.getName() + " target").str(), OS);
   std::vector<RuleMatcher> Rules;
   // Look through the SelectionDAG patterns we found, possibly emitting some.
void GlobalISelEmitter::run(raw_ostream &OS) {
   OS << "// ComplexPattern predicates.\n"
      << "enum {\n"
      << "  GICP_Invalid,\n";
   for (const auto &Record : ComplexPredicates)
     OS << "  GICP_" << Record->getName() << ",\n";
   OS << "};\n"
      << "// See constructor for table contents\n\n";
 
-  // Emit imm predicate table and an enum to reference them with.
-  // The 'Predicate_' part of the name is redundant but eliminating it is more
-  // trouble than it's worth.
-  {
-    OS << "// PatFrag predicates.\n"
-       << "enum {\n";
-    StringRef EnumeratorSeparator = " = GIPFP_Invalid + 1,\n";
-    for (const auto *Record : RK.getAllDerivedDefinitions("PatFrag")) {
-      if (!Record->getValueAsString("ImmediateCode").empty()) {
-        OS << "  GIPFP_Predicate_" << Record->getName() << EnumeratorSeparator;
-        EnumeratorSeparator = ",\n";
-      }
-    }
-    OS << "};\n";
-  }
-  for (const auto *Record : RK.getAllDerivedDefinitions("PatFrag"))
-    if (!Record->getValueAsString("ImmediateCode").empty())
-      OS << "  static bool Predicate_" << Record->getName() << "(int64_t Imm) {"
-         << Record->getValueAsString("ImmediateCode") << "  }\n";
-  OS << "static InstructionSelector::ImmediatePredicateFn ImmPredicateFns[] = "
-        "{\n"
-     << "  nullptr,\n";
-  for (const auto *Record : RK.getAllDerivedDefinitions("PatFrag"))
-    if (!Record->getValueAsString("ImmediateCode").empty())
-      OS << "  Predicate_" << Record->getName() << ",\n";
-  OS << "};\n";
+  emitImmPredicates(OS, [](const Record *R) {
+    bool Unset;
+    return !R->getValueAsBitOrUnset("IsAPFloat", Unset) &&
+           !R->getValueAsBit("IsAPInt");
+  });
 
   OS << "bool " << Target.getName()
      << "InstructionSelector::selectImpl(MachineInstr &I) const {\n"
      << "  MachineFunction &MF = *I.getParent()->getParent();\n"
      << "  MachineRegisterInfo &MRI = MF.getRegInfo();\n"
      << "  // FIXME: This should be computed on a per-function basis rather "
         "than per-insn.\n"
      << "  AvailableFunctionFeatures = computeAvailableFunctionFeatures(&STI, "