[PATCH] D100691: [TableGen] Store predicates in PatternToMatch as ListInit *. Add string for HwModeFeatures

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:4370
-
-      // Add negations of the HM's predicates to the default predicate.
-      DefaultPred.emplace_back(Predicate(HM.Features, false));
----------------
craig.topper wrote:
> kparzysz wrote:
> > Is this part removed in your patch?  The "default" case could be vacuously true if there are no conditions attached to it, and it could be applied when a more specialized case should be applied instead.
> > The "default" was originally invented for situations where the specific modes don't cover all possibilities when combined.  I guess in practice the "default" ends up being one of the explicitly defined modes, so this "accidental application" may not be happening.
> I removed it because it wasn't doing what the comment said, the DefaultPred vector is only pushed to it's not used otherwise. Since it was incomplete, I didn't know how to update it.
Yeah, see the comment below.  It's a latent bug, unfortunately.

I added this change on top of the current main branch (i.e. without your changes) and both `check-llvm` and `check-clang` passed for all targets.  I guess it hasn't caused trouble for the reasons I stated, but it could eventually.  Another variant of the intent described above was to make sure that the predicated patterns can be tried in any order without affecting the result.

I'm not sure how you want to proceed.  There is always the possibility of changing the initial intent if it doesn't break the current code...


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:4382
     if (HasDefault)
       AppendPattern(P, DefaultMode);
   }
----------------
This should be
```
if (HasDefault) {
  ModeChecks[DefaultMode] = DefaultPred;
  AppendPattern(P, DefaultMode);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100691