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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83034