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

[Madhur Amilkanthwar via llvm-commits]

Ping!

On Tue, Jul 21, 2020, 7:42 PM Madhur Amilkanthwar via Phabricator <
reviews at reviews.llvm.org> wrote:

> madhur13490 marked an inline comment 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:
> > > > 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.
> 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;
>     }}
>
> ```
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D83034/new/
>
> https://reviews.llvm.org/D83034
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200724/497ddd73/attachment.html>