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

Tue Aug 4 04:36:59 PDT 2020

Ping!

On Fri, Jul 31, 2020 at 10:37 AM Madhur Amilkanthwar <madhur13490 at gmail.com>
wrote:

> Ping!
>
> On Mon, Jul 27, 2020, 9:26 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:
>> > madhur13490 wrote:
>> > > 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;
>> > >     }}
>> > >
>> > > ```
>> > I believe we're describing the same thing. My suggestion left it at:
>> > ```
>> >   for (const TreePredicateCall &Call : Src->getPredicateCalls()) {
>> >     const TreePredicateFn &Predicate = Call.Fn;
>> >     if (Predicate.isAlwaysTrue())
>> >       continue;
>> >
>> >     if (Predicate.isImmediatePattern()) {
>> >
>>  InsnMatcher.addPredicate<InstructionImmPredicateMatcher>(Predicate);
>> >       continue;
>> >     }
>> >
>> >     addBuiltinPredicate(...);
>> >
>> >     if (Predicate.hasGISelPredicateCode()) {
>> >
>>  InsnMatcher.addPredicate<GenericInstructionPredicateMatcher>(Predicate);
>> >       continue;
>> >     }
>> >
>> >     return failedImport("Src pattern child has predicate (" +
>> >                         explainPredicates(Src) + ")");
>> >   }
>> > ```
>> > the code that was extracted into addBuiltinPredicate() then does
>> `return` wherever it currently does `continue`.
>> >
>> > > The loop I am referring to is  ` for (const TreePredicateCall &Call :
>> Src->getPredicateCalls()) {` at line 3604 in the original code.
>> > I'm not sure we're using the same line numbers here. 3604 is:
>> > ```
>> >       continue;
>> > ```
>> > in the original code and
>> > ```
>> >           0, MemoryVsLLTSizePredicateMatcher::EqualTo, 0);
>> > ```
>> > in the new code according to phabricator.
>> Yeah, we're not referring to same line numbers :)
>> But anyway as we're on the same page, I updated the change to reflect.
>>
>>
>> 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/20200804/dc7d74ba/attachment.html>