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

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 marked 2 inline comments as done.
madhur13490 added inline comments.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575
+      HasPredicateCode = true;
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+    }
----------------
dsanders wrote:
> dsanders wrote:
> > 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
> Also, just to confirm I agree it should be added after the builtins. Otherwise your C++ can't assume anything covered by the builtins.
Is it just for "assumption" or it would affect the functionality if custom predicates are added later? 


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:3575
+      HasPredicateCode = true;
+      InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
+    }
----------------
madhur13490 wrote:
> dsanders wrote:
> > dsanders wrote:
> > > 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
> > Also, just to confirm I agree it should be added after the builtins. Otherwise your C++ can't assume anything covered by the builtins.
> Is it just for "assumption" or it would affect the functionality if custom predicates are added later? 
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83034