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

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 marked an inline comment as done.
madhur13490 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. 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.



================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575
+      HasPredicateCode = true;
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+    }
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83034