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

[Madhur Amilkanthwar via llvm-commits]

Ping!

On Thu, Jul 16, 2020 at 2:19 PM Madhur Amilkanthwar via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>

-- 
*Disclaimer: Views, concerns, thoughts, questions, ideas expressed in this
mail are of my own and my employer has no take in it. *
Thank You.
Madhur D. Amilkanthwar
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200720/634209ca/attachment.html>