[GlobalISel] Don't skip adding predicate matcher

[madhur13490]

Summary

This patch fixes a bug which skipped
adding predicate matcher for a pattern in many cases.
For example, if predicate is Load and
its memoryVT is non-null then the loop
continues and never reaches to the end which
adds the predicate matcher. This patch moves the
matcher addition to the top of the loop
so that it gets added regardless of contextual checks
later in the loop.
Other way to fix this issue is to remove all "continue" statements
in checks and let the loop continue till end.

[madhur13490]

Fix test failures

[nhaehnle]

Could you provide a more concrete example that goes wrong without this change? I believe that could help understanding, would be good to add to the commit message for this.

[madhur13490]

Ping!

[arsenm]

I would expect the existing tablegen tests to break from this and need updating?

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

I think this should remain as the last predicate type added to the matcher, after everything else here. The ordering does matter (see D82331)

[dsanders]

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

llvm/utils/TableGen/GlobalISelEmitter.cpp

3637–3647

These two cases covered all of the loads with predicates that were moved to built-in predicates.

[arsenm]

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns. However, the PatFrags are much more free form and you can define a patfrag that combines multiple of these predicates in the same "layer" of the load/store hierarchy. The AMDGPU patterns redefine the entire set of load/store patterns, and in some cases it makes sense to combine all the predicates at once. I had to somewhat artificially add new layers of PatFrags to only apply a single predicate at a time. This also isn't consistently applied (for example, it does work to combine the AddressSpaces predicate and alignment)

[madhur13490]

In D83034#2133989, @arsenm wrote:

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns. However, the PatFrags are much more free form and you can define a patfrag that combines multiple of these predicates in the same "layer" of the load/store hierarchy. The AMDGPU patterns redefine the entire set of load/store patterns, and in some cases it makes sense to combine all the predicates at once. I had to somewhat artificially add new layers of PatFrags to only apply a single predicate at a time. This also isn't consistently applied (for example, it does work to combine the AddressSpaces predicate and alignment)

Yes in addition to what Matt explained here, I believe the custom predicates should be allowed to use on any patterns because we cannot always guarantee that everything can be covered in other checks. Sometimes, we may want to do complicated checks for which custom predicates makes more sense that adding a builtin for each sub-checks. FWIW, the function needs refactoring because it does checks like isAtomic() and isLoad() several times. We can really do these checks once and then add an appropriate checker. The function seems tailored to a class of instructions but the name of the function seems generic.

I have added a test.

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

Well, then we have to remove all "continue" statements. Are we fine with it? I always thought that we'll use custom predicates for things which are *not* covered in other checks. I am fine with keeping it at the end, just looking for consequences.

[madhur13490]

rebase + add test

[madhur13490]

In D83034#2133027, @arsenm wrote:

I would expect the existing tablegen tests to break from this and need updating?

No failures.

[dsanders]

In D83034#2133989, @arsenm wrote:

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns.

That's because the builtin ones implement the predicates from the PatFrag hierarchy that was in the Target.td loads. Those PatFrags resulted in three distinct predicates applied to the same node on most load/stores. That's what I mean when I say mixing builtin/custom wasn't intended. The builtins were only intended to replace that specific hierarchy. Once we had C++ predicates there wasn't an apparent need to extend the builtins any further.

However, the PatFrags are much more free form and you can define a patfrag that combines multiple of these predicates in the same "layer" of the load/store hierarchy. The AMDGPU patterns redefine the entire set of load/store patterns, and in some cases it makes sense to combine all the predicates at once. I had to somewhat artificially add new layers of PatFrags to only apply a single predicate at a time.

Yep, I don't see a reason why you shouldn't be allowed to use these in different ways. I'm just saying that wasn't intended when they were added. The expectation was that other predicates would continue to use C++ predicates as they did before. There are some benefits to factoring out the portions of the C++ that are equivalent to these predicates though (the generated state machine can make use of this information when it's optimized) which is partly why I think it ought to be allowed.

This also isn't consistently applied (for example, it does work to combine the AddressSpaces predicate and alignment)

IIRC, that's because there was originally a C++ predicate that checked both together. I think it was target specific rather than Target.td though

In D83034#2147553, @madhur13490 wrote:

In D83034#2133989, @arsenm wrote:

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns. However, the PatFrags are much more free form and you can define a patfrag that combines multiple of these predicates in the same "layer" of the load/store hierarchy. The AMDGPU patterns redefine the entire set of load/store patterns, and in some cases it makes sense to combine all the predicates at once. I had to somewhat artificially add new layers of PatFrags to only apply a single predicate at a time. This also isn't consistently applied (for example, it does work to combine the AddressSpaces predicate and alignment)

Yes in addition to what Matt explained here, I believe the custom predicates should be allowed to use on any patterns because we cannot always guarantee that everything can be covered in other checks. Sometimes, we may want to do complicated checks for which custom predicates makes more sense that adding a builtin for each sub-checks.

I'm not sure I'm following this. Custom predicates can be used on any patterns. What I'm saying is that you can't (currently) have a single PatFrag that is simultaneously a builtin and a custom predicate. They have to be distinct PatFrags at the moment (i.e. a PatFrag with a custom predicate that matches another PatFrag with a builtin predicate) like how sextloadi32 contains sextload which contains unindexedload. These three add distinct predicates to form the three predicates on one node that we had in the internal representation of the pattern.

FWIW, the function needs refactoring because it does checks like isAtomic() and isLoad() several times. We can really do these checks once and then add an appropriate checker. The function seems tailored to a class of instructions but the name of the function seems generic.

The alternatives I tried at the time generally ended up more confusing. The current builtins code is generally structured as:

if (isBuiltin1()) {
  addBuiltin1();
  continue;
}
if (isBuiltin2()) {
  addBuiltin2();
  continue;
}

but the isBuiltinN()'s were split into subpredicates because the MemVT's made the number of specific predicates excessive and because the predicates left over after extracting that left a fair bit of code duplication.
I do think it should be hoisted into a function though.

I have added a test.

Thanks. That confirmed my understanding of the issue

llvm/test/TableGen/ContextlessPredicates.td

14–16

Thanks, this confirms what I was thinking. This PatFrag is simultaneously a builtin and a custom which wasn't really intended but should probably be allowed (even if it wasn't at the very least it should error, silently dropping it is not ok). The equivalent that fits into the original intent is:

def atomic_swap_i32 : PatFrag<(ops node:$ptr, node:$val),
                                               (atomic_swap node:$ptr, node:$val)> {
  let IsAtomic = 1;
  let MemoryVT = i32;
}
def test_atomic_op_frag : PatFrag<(ops node:$ptr, node:$val),
                                               (atomic_swap_i32 node:$ptr, node:$val)> {
  let GISelPredicateCode = [{ dbgs() <<  return !MRI.use_nodbg_empty(MI.getOperand(0).getReg()); }];
}

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

I'd be inclined to factor the builtins out into a function instead so they can return and then you resume at the code to check+add for custom predicates

[dsanders]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

Also, just to confirm I agree it should be added after the builtins. Otherwise your C++ can't assume anything covered by the builtins.

[madhur13490]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

Is it just for "assumption" or it would affect the functionality if custom predicates are added later?

3764

Sure, I am up for refactoring, but can you please explain a bit more, how would you that refactoring look? I was more inclined to rewriting the loop which iterates over PredicateCalls and add matcher. One of the many ways in which we can refactor the loop is like

if (isAtomic())
  if (isLoad())
   add all matchers
...

but this may change the order in which matchers are added (which brings me back to the question - is the order important?). I can further outline this in a static function.
But this is a bit of design question.

So, I'd like to see what you're imagining in a form of pseudo code :)

[dsanders]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

Suppose the builtin checks it's a i32 non-extending load and the custom predicate wants to check the address generation. If the custom predicate is first then it needs to check the following pseudo-code:

isLoad() && isNonExtending() && isMMO32bit() && checkAddrGen(...)

whereas if the builtin is first then the first three components are tested by the builtin first so the custom predicate is:

checkAddrGen(...)

3764

Sure, I am up for refactoring, but can you please explain a bit more, how would you that refactoring look?

What I had in mind was:

• Extract lines 3572 to 3707 of the original code into a function, passing parameters by value or by reference as appropriate
• Replace all the continues with return

Then after those predicates are added it will resume execution from line 3709 and do the custom predicates

which brings me back to the question - is the order important?

In general, the order predicates are tested does matter. For example, testing an MMO property (e.g. the MemoryVT) without knowing that an MMO is there will lead to a crash. Specific pairs might be reorderable but unless there's a strong need to change the current order and re-verify the new order works for all targets, I'd preserve it.

[madhur13490]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

With your suggested way, we'd be extracting few statements of loop in the function but for rest of the loop body, we'll still have to run the loop in caller i.e. current function. So, we'd be running the loop twice which seems time consuming to me. Line 3707 belongs to this loop. The loop I am referring to is for (const TreePredicateCall &Call : Src->getPredicateCalls()) { at line 3604 in the original code.

Are you sure about this?

I was thinking of just refactoring the loop something like

for (const TreePredicateCall &Call : Src->getPredicateCalls()) {  
     addBuiltinPredicate(); // new function
     if (Predicate.hasGISelPredicateCode()) {
      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
      continue;
    }}

[dsanders]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

I believe we're describing the same thing. My suggestion left it at:

for (const TreePredicateCall &Call : Src->getPredicateCalls()) {
  const TreePredicateFn &Predicate = Call.Fn;
  if (Predicate.isAlwaysTrue())
    continue;

  if (Predicate.isImmediatePattern()) {
    InsnMatcher.addPredicate<InstructionImmPredicateMatcher>(Predicate);
    continue;
  }

  addBuiltinPredicate(...);

  if (Predicate.hasGISelPredicateCode()) {
    InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
    continue;
  }

  return failedImport("Src pattern child has predicate (" +
                      explainPredicates(Src) + ")");
}

the code that was extracted into addBuiltinPredicate() then does return wherever it currently does continue.

The loop I am referring to is for (const TreePredicateCall &Call : Src->getPredicateCalls()) { at line 3604 in the original code.

I'm not sure we're using the same line numbers here. 3604 is:

continue;

in the original code and

0, MemoryVsLLTSizePredicateMatcher::EqualTo, 0);

in the new code according to phabricator.

[madhur13490]

rebase + address comments

[madhur13490]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3764

Yeah, we're not referring to same line numbers :)
But anyway as we're on the same page, I updated the change to reflect.

[madhur13490]

@arsenm , Can you please take over? I think @dsanders is on vacation.

[arsenm]

llvm/test/TableGen/ContextlessPredicates.td

13

Don't need dbgs() << here (not that you really need anything)

llvm/utils/TableGen/GlobalISelEmitter.cpp

3665

How is changing this to return any different than continue? It's still treating all of these as mutually exclusive

3770

No else after continue

[madhur13490]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3665

It is returning to the parent createAndImportSelDAGMatcher which further adds custom predicate.

[madhur13490]

rebase + address comments

[madhur13490]

fix formatting

[madhur13490]

fix unexpected tabs

[arsenm]

LGTM. I think this is only a half solution, and it's still a problem that multiple builtin predicates will be silently ignored

llvm/utils/TableGen/GlobalISelEmitter.cpp

3665

OK, so this is *only* fixing custom predicate + builtin predicate. It is not fixing adding multiple builtin predicates at once. I think that can be a separate patch, but is just as broken

[madhur13490]

In D83034#2224104, @arsenm wrote:

LGTM. I think this is only a half solution, and it's still a problem that multiple builtin predicates will be silently ignored

I agree. Will fix soon.

llvm/test/TableGen/ContextlessPredicates.td

+// RUN: llvm-tblgen -gen-global-isel -I %p/../../include -I %p/Common -optimize-match-table=false %s -o %T/context-non-optimized.cpp
+// RUN: FileCheck %s --check-prefixes=CHECK_NOPT -input-file=%T/context-non-optimized.cpp
+// RUN: llvm-tblgen -gen-global-isel -I %p/../../include -I %p/Common -optimize-match-table=true  %s -o %T/context-optimized.cpp
+// RUN: FileCheck %s --check-prefixes=CHECK_OPT -input-file=%T/context-optimized.cpp
+
+
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def test_atomic_op_frag : PatFrag<(ops node:$ptr, node:$val),
+                                               (atomic_swap node:$ptr, node:$val)> {
+  let GISelPredicateCode = [{ dbgs() <<  return !MRI.use_nodbg_empty(MI.getOperand(0).getReg()); }];
+  let IsAtomic = 1;
+  let MemoryVT = i32;
+}
+ 
+def INSN : I<(outs GPR32:$dst), (ins GPR32Op:$src1, GPR32Op:$src2), []>;
+
+def : Pat<(test_atomic_op_frag GPR32:$ptr, GPR32:$val) ,
+        (INSN GPR32:$ptr, GPR32:$val)>;
+
+// CHECK_NOPT-LABEL: const int64_t *MyTargetInstructionSelector::getMatchTable() const {
+// CHECK_NOPT-NEXT:  constexpr static int64_t MatchTable0[] = {
+// CHECK_NOPT-NEXT:    GIM_Try, /*On fail goto*//*Label 0*/ 46, // Rule ID 0 //
+// CHECK_NOPT-NEXT:      GIM_CheckNumOperands, /*MI*/0, /*Expected*/3,
+// CHECK_NOPT-NEXT:      GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ATOMICRMW_XCHG,
+// CHECK_NOPT-NEXT:      GIM_CheckMemorySizeEqualTo, /*MI*/0, /*MMO*/0, /*Size*/4,
+// CHECK_NOPT-NEXT:      // MIs[0] dst
+// CHECK_NOPT-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK_NOPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_NOPT-NEXT:      // MIs[0] ptr
+// CHECK_NOPT-NEXT:      GIM_CheckPointerToAny, /*MI*/0, /*Op*/1, /*SizeInBits*/32,
+// CHECK_NOPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_NOPT-NEXT:      // MIs[0] val
+// CHECK_NOPT-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// CHECK_NOPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/2, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_NOPT-NEXT:      GIM_CheckCxxInsnPredicate, /*MI*/0, /*FnId*/GIPFP_MI_Predicate_test_atomic_op_frag,
+// CHECK_NOPT-NEXT:      // (atomic_swap:{ *:[i32] } GPR32:{ *:[i32] }:$ptr, GPR32:{ *:[i32] }:$val)<<P:Predicate_test_atomic_op_frag>>  =>  (INSN:{ *:[i32] } GPR32:{ *:[i32] }:$ptr, GPR32:{ *:[i32] }:$val)
+// CHECK_NOPT-NEXT:      GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::INSN,
+// CHECK_NOPT-NEXT:      GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK_NOPT-NEXT:      // GIR_Coverage, 0,
+// CHECK_NOPT-NEXT:      GIR_Done,
+// CHECK_NOPT-NEXT:    // Label 0: @46
+// CHECK_NOPT-NEXT:    GIM_Reject,
+// CHECK_NOPT-NEXT:    };
+// CHECK_NOPT-NEXT:  return MatchTable0;
+// CHECK_NOPT-NEXT: }
+//
+//
+
+// CHECK_OPT-LABEL: const int64_t *MyTargetInstructionSelector::getMatchTable() const {
+// CHECK_OPT-NEXT:  constexpr static int64_t MatchTable0[] = {
+// CHECK_OPT-NEXT:    GIM_Try, /*On fail goto*//*Label 0*/ 43, // Rule ID 0 //
+// CHECK_OPT-NEXT:      GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ATOMICRMW_XCHG,
+// CHECK_OPT-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK_OPT-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// CHECK_OPT-NEXT:      GIM_CheckMemorySizeEqualTo, /*MI*/0, /*MMO*/0, /*Size*/4,
+// CHECK_OPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_OPT-NEXT:      // MIs[0] ptr
+// CHECK_OPT-NEXT:      GIM_CheckPointerToAny, /*MI*/0, /*Op*/1, /*SizeInBits*/32,
+// CHECK_OPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_OPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/2, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK_OPT-NEXT:      GIM_CheckCxxInsnPredicate, /*MI*/0, /*FnId*/GIPFP_MI_Predicate_test_atomic_op_frag,
+// CHECK_OPT-NEXT:      // (atomic_swap:{ *:[i32] } GPR32:{ *:[i32] }:$ptr, GPR32:{ *:[i32] }:$val)<<P:Predicate_test_atomic_op_frag>>  =>  (INSN:{ *:[i32] } GPR32:{ *:[i32] }:$ptr, GPR32:{ *:[i32] }:$val)
+// CHECK_OPT-NEXT:      GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::INSN,
+// CHECK_OPT-NEXT:      GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK_OPT-NEXT:      // GIR_Coverage, 0,
+// CHECK_OPT-NEXT:      GIR_Done,
+// CHECK_OPT-NEXT:    // Label 0: @43
+// CHECK_OPT-NEXT:    GIM_Reject,
+// CHECK_OPT-NEXT:    };
+// CHECK_OPT-NEXT:  return MatchTable0;
+// CHECK_OPT-NEXT: }

llvm/utils/TableGen/GlobalISelEmitter.cpp

private:
   Optional<const CodeGenRegisterClass *>
   getRegClassFromLeaf(TreePatternNode *Leaf);
 
   /// Return a CodeGenRegisterClass for \p N if one can be found. Return None
   /// otherwise.
   Optional<const CodeGenRegisterClass *>
   inferRegClassFromPattern(TreePatternNode *N);
 
+  // Add builtin predicates.
+  Expected<InstructionMatcher &>
+  addBuiltinPredicates(const Record *SrcGIEquivOrNull,
+                       const TreePredicateFn &Predicate,
+                       InstructionMatcher &InsnMatcher, bool &HasAddedMatcher);
+
 public:
   /// Takes a sequence of \p Rules and group them based on the predicates
   /// they share. \p MatcherStorage is used as a memory container
   /// for the group that are created as part of this process.
   ///
   /// What this optimization does looks like if GroupT = GroupMatcher:
   /// Output without optimization:
   /// \verbatim
if (!P.Def || P.getCondString().empty())
       continue;
     declareSubtargetFeature(P.Def);
     M.addRequiredFeature(P.Def);
   }
 
   return Error::success();
 }
 
-Expected<InstructionMatcher &> GlobalISelEmitter::createAndImportSelDAGMatcher(
-    RuleMatcher &Rule, InstructionMatcher &InsnMatcher,
-    const TreePatternNode *Src, unsigned &TempOpIdx) {
-  Record *SrcGIEquivOrNull = nullptr;
-  const CodeGenInstruction *SrcGIOrNull = nullptr;
-
-  // Start with the defined operands (i.e., the results of the root operator).
-  if (Src->getExtTypes().size() > 1)
-    return failedImport("Src pattern has multiple results");
-
-  if (Src->isLeaf()) {
-    Init *SrcInit = Src->getLeafValue();
-    if (isa<IntInit>(SrcInit)) {
-      InsnMatcher.addPredicate<InstructionOpcodeMatcher>(
-          &Target.getInstruction(RK.getDef("G_CONSTANT")));
-    } else
-      return failedImport(
-          "Unable to deduce gMIR opcode to handle Src (which is a leaf)");
-  } else {
-    SrcGIEquivOrNull = findNodeEquiv(Src->getOperator());
-    if (!SrcGIEquivOrNull)
-      return failedImport("Pattern operator lacks an equivalent Instruction" +
-                          explainOperator(Src->getOperator()));
-    SrcGIOrNull = getEquivNode(*SrcGIEquivOrNull, Src);
-
-    // The operators look good: match the opcode
-    InsnMatcher.addPredicate<InstructionOpcodeMatcher>(SrcGIOrNull);
-  }
-
-  unsigned OpIdx = 0;
-  for (const TypeSetByHwMode &VTy : Src->getExtTypes()) {
-    // Results don't have a name unless they are the root node. The caller will
-    // set the name if appropriate.
-    OperandMatcher &OM = InsnMatcher.addOperand(OpIdx++, "", TempOpIdx);
-    if (auto Error = OM.addTypeCheckPredicate(VTy, false /* OperandIsAPointer */))
-      return failedImport(toString(std::move(Error)) +
-                          " for result of Src pattern operator");
-  }
-
-  for (const TreePredicateCall &Call : Src->getPredicateCalls()) {
-    const TreePredicateFn &Predicate = Call.Fn;
-    if (Predicate.isAlwaysTrue())
-      continue;
-
-    if (Predicate.isImmediatePattern()) {
-      InsnMatcher.addPredicate<InstructionImmPredicateMatcher>(Predicate);
-      continue;
-    }
-
-    // An address space check is needed in all contexts if there is one.
-    if (Predicate.isLoad() || Predicate.isStore() || Predicate.isAtomic()) {
-      if (const ListInit *AddrSpaces = Predicate.getAddressSpaces()) {
-        SmallVector<unsigned, 4> ParsedAddrSpaces;
+Expected<InstructionMatcher &> GlobalISelEmitter::addBuiltinPredicates(
+    const Record *SrcGIEquivOrNull, const TreePredicateFn &Predicate,
+    InstructionMatcher &InsnMatcher, bool &HasAddedMatcher) {
 
-        for (Init *Val : AddrSpaces->getValues()) {
-          IntInit *IntVal = dyn_cast<IntInit>(Val);
-          if (!IntVal)
-            return failedImport("Address space is not an integer");
-          ParsedAddrSpaces.push_back(IntVal->getValue());
-        }
 
-        if (!ParsedAddrSpaces.empty()) {
-          InsnMatcher.addPredicate<MemoryAddressSpacePredicateMatcher>(
             0, ParsedAddrSpaces);
-        }
-      }
 
-      int64_t MinAlign = Predicate.getMinAlignment();
-      if (MinAlign > 0)
-        InsnMatcher.addPredicate<MemoryAlignmentPredicateMatcher>(0, MinAlign);
-    }
 
-    // G_LOAD is used for both non-extending and any-extending loads.
-    if (Predicate.isLoad() && Predicate.isNonExtLoad()) {
-      InsnMatcher.addPredicate<MemoryVsLLTSizePredicateMatcher>(
-          0, MemoryVsLLTSizePredicateMatcher::EqualTo, 0);
-      continue;
-    }
-    if (Predicate.isLoad() && Predicate.isAnyExtLoad()) {
-      InsnMatcher.addPredicate<MemoryVsLLTSizePredicateMatcher>(
-          0, MemoryVsLLTSizePredicateMatcher::LessThan, 0);
-      continue;
-    }
+    return InsnMatcher;
+    return InsnMatcher;
 
-    if (Predicate.isStore()) {
-      if (Predicate.isTruncStore()) {
-        // FIXME: If MemoryVT is set, we end up with 2 checks for the MMO size.
-        InsnMatcher.addPredicate<MemoryVsLLTSizePredicateMatcher>(
-            0, MemoryVsLLTSizePredicateMatcher::LessThan, 0);
-        continue;
-      }
-      if (Predicate.isNonTruncStore()) {
-        // We need to check the sizes match here otherwise we could incorrectly
-        // match truncating stores with non-truncating ones.
-        InsnMatcher.addPredicate<MemoryVsLLTSizePredicateMatcher>(
-            0, MemoryVsLLTSizePredicateMatcher::EqualTo, 0);
-      }
-    }
+      return InsnMatcher;
 
-    // No check required. We already did it by swapping the opcode.
-    if (!SrcGIEquivOrNull->isValueUnset("IfSignExtend") &&
-        Predicate.isSignExtLoad())
-      continue;
+    return InsnMatcher;
 
-    // No check required. We already did it by swapping the opcode.
-    if (!SrcGIEquivOrNull->isValueUnset("IfZeroExtend") &&
-        Predicate.isZeroExtLoad())
-      continue;
+    return InsnMatcher;
 
-    // No check required. G_STORE by itself is a non-extending store.
-    if (Predicate.isNonTruncStore())
-      continue;
+    return InsnMatcher;
 
-    if (Predicate.isLoad() || Predicate.isStore() || Predicate.isAtomic()) {
-      if (Predicate.getMemoryVT() != nullptr) {
-        Optional<LLTCodeGen> MemTyOrNone =
-            MVTToLLT(getValueType(Predicate.getMemoryVT()));
 
-        if (!MemTyOrNone)
-          return failedImport("MemVT could not be converted to LLT");
 
-        // MMO's work in bytes so we must take care of unusual types like i1
-        // don't round down.
-        unsigned MemSizeInBits =
-            llvm::alignTo(MemTyOrNone->get().getSizeInBits(), 8);
 
-        InsnMatcher.addPredicate<MemorySizePredicateMatcher>(
-            0, MemSizeInBits / 8);
-        continue;
-      }
-    }
+      InsnMatcher.addPredicate<MemorySizePredicateMatcher>(0,
+                                                           MemSizeInBits / 8);
+      return InsnMatcher;
 
-    if (Predicate.isLoad() || Predicate.isStore()) {
-      // No check required. A G_LOAD/G_STORE is an unindexed load.
-      if (Predicate.isUnindexed())
-        continue;
-    }
+      return InsnMatcher;
 
-    if (Predicate.isAtomic()) {
-      if (Predicate.isAtomicOrderingMonotonic()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "Monotonic");
-        continue;
-      }
-      if (Predicate.isAtomicOrderingAcquire()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>("Acquire");
-        continue;
-      }
-      if (Predicate.isAtomicOrderingRelease()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>("Release");
-        continue;
-      }
-      if (Predicate.isAtomicOrderingAcquireRelease()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "AcquireRelease");
-        continue;
-      }
-      if (Predicate.isAtomicOrderingSequentiallyConsistent()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "SequentiallyConsistent");
-        continue;
-      }
+      InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>("Monotonic");
+      return InsnMatcher;
+      return InsnMatcher;
+      return InsnMatcher;
+      return InsnMatcher;
+      return InsnMatcher;
+    }
 
-      if (Predicate.isAtomicOrderingAcquireOrStronger()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "Acquire", AtomicOrderingMMOPredicateMatcher::AO_OrStronger);
-        continue;
-      }
-      if (Predicate.isAtomicOrderingWeakerThanAcquire()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "Acquire", AtomicOrderingMMOPredicateMatcher::AO_WeakerThan);
-        continue;
-      }
+    return InsnMatcher;
+    return InsnMatcher;
 
-      if (Predicate.isAtomicOrderingReleaseOrStronger()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "Release", AtomicOrderingMMOPredicateMatcher::AO_OrStronger);
-        continue;
-      }
-      if (Predicate.isAtomicOrderingWeakerThanRelease()) {
-        InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
-            "Release", AtomicOrderingMMOPredicateMatcher::AO_WeakerThan);
-        continue;
-      }
-    }
+    return InsnMatcher;
+    return InsnMatcher;
+  HasAddedMatcher = false;
+  return InsnMatcher;
 
-    if (Predicate.hasGISelPredicateCode()) {
-      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+Expected<InstructionMatcher &> GlobalISelEmitter::createAndImportSelDAGMatcher(
+    RuleMatcher &Rule, InstructionMatcher &InsnMatcher,
+    const TreePatternNode *Src, unsigned &TempOpIdx) {
+  Record *SrcGIEquivOrNull = nullptr;
+  const CodeGenInstruction *SrcGIOrNull = nullptr;
+
+  // Start with the defined operands (i.e., the results of the root operator).
+  if (Src->getExtTypes().size() > 1)
+    return failedImport("Src pattern has multiple results");
+
+  if (Src->isLeaf()) {
+    Init *SrcInit = Src->getLeafValue();
+    if (isa<IntInit>(SrcInit)) {
+      InsnMatcher.addPredicate<InstructionOpcodeMatcher>(
+          &Target.getInstruction(RK.getDef("G_CONSTANT")));
+    } else
+      return failedImport(
+          "Unable to deduce gMIR opcode to handle Src (which is a leaf)");
+  } else {
+    SrcGIEquivOrNull = findNodeEquiv(Src->getOperator());
+    if (!SrcGIEquivOrNull)
+      return failedImport("Pattern operator lacks an equivalent Instruction" +
+                          explainOperator(Src->getOperator()));
+    SrcGIOrNull = getEquivNode(*SrcGIEquivOrNull, Src);
+
+    // The operators look good: match the opcode
+    InsnMatcher.addPredicate<InstructionOpcodeMatcher>(SrcGIOrNull);
+  }
+
+  unsigned OpIdx = 0;
+  for (const TypeSetByHwMode &VTy : Src->getExtTypes()) {
+    // Results don't have a name unless they are the root node. The caller will
+    // set the name if appropriate.
+    OperandMatcher &OM = InsnMatcher.addOperand(OpIdx++, "", TempOpIdx);
+    if (auto Error = OM.addTypeCheckPredicate(VTy, false /* OperandIsAPointer */))
+      return failedImport(toString(std::move(Error)) +
+                          " for result of Src pattern operator");
+  }
+
+  for (const TreePredicateCall &Call : Src->getPredicateCalls()) {
+    const TreePredicateFn &Predicate = Call.Fn;
+    bool HasAddedBuiltinMatcher = true;
+    if (Predicate.isAlwaysTrue())
+      continue;
+
+    if (Predicate.isImmediatePattern()) {
+      InsnMatcher.addPredicate<InstructionImmPredicateMatcher>(Predicate);
       continue;
     }
 
+    auto InsnMatcherOrError = addBuiltinPredicates(
+        SrcGIEquivOrNull, Predicate, InsnMatcher, HasAddedBuiltinMatcher);
+    if (auto Error = InsnMatcherOrError.takeError())
+      return std::move(Error);
+
+    if (Predicate.hasGISelPredicateCode()) {
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+      continue;
+    } else if (!HasAddedBuiltinMatcher) {
       return failedImport("Src pattern child has predicate (" +
                           explainPredicates(Src) + ")");
     }
+  }
   if (SrcGIEquivOrNull && SrcGIEquivOrNull->getValueAsBit("CheckMMOIsNonAtomic"))
     InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>("NotAtomic");
   else if (SrcGIEquivOrNull && SrcGIEquivOrNull->getValueAsBit("CheckMMOIsAtomic")) {
     InsnMatcher.addPredicate<AtomicOrderingMMOPredicateMatcher>(
       "Unordered", AtomicOrderingMMOPredicateMatcher::AO_OrStronger);
   }
 
   if (Src->isLeaf()) {