[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83034