[PATCH] D83034: [GlobalISel] Don't skip adding predicate matcher

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D83034#2133989 <https://reviews.llvm.org/D83034#2133989>, @arsenm wrote:

> In D83034#2133375 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D83034#2147553>, @madhur13490 wrote:

> In D83034#2133989 <https://reviews.llvm.org/D83034#2133989>, @arsenm wrote:
>
> > In D83034#2133375 <https://reviews.llvm.org/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



================
Comment at: llvm/test/TableGen/ContextlessPredicates.td:13-15
+  let GISelPredicateCode = [{ dbgs() <<  return !MRI.use_nodbg_empty(MI.getOperand(0).getReg()); }];
+  let IsAtomic = 1;
+  let MemoryVT = i32;
----------------
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()); }];
}
```


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575
+      HasPredicateCode = true;
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+    }
----------------
madhur13490 wrote:
> arsenm wrote:
> > I think this should remain as the last predicate type added to the matcher, after everything else here. The ordering does matter (see D82331)
> 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. 
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83034/new/

https://reviews.llvm.org/D83034