[PATCH] D66871: [SVE] MVT scalable size queries

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.

I don't see anything else wrong with this. LGTM if you rename the LE predicate.



================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:520
     // to a non-vector, it should return false (to avoid removal).
     if (A.isVector() != B.isVector())
       return false;
----------------
huntergr wrote:
> huntergr wrote:
> > huntergr wrote:
> > > rovka wrote:
> > > > Why does LE care about isVector and LT doesn't?
> > > > 
> > > > In any case, I think both functions would benefit from being rewritten using std::tie and the corresponding operators.
> > > LT seems to be used with filtered min/max functions below and will only operate on either only vector or only scalar MVTs, so never compares between them.
> > > 
> > > LE is then explicitly used to remove MVTs from Typesets that may have mixed scalar and vector MVTs, so needs to avoid comparing them.
> > > 
> > > I'll try reworking them with std::tie.
> > I've reworked them to use `std::tie`, which meant I had to make the size query methods return `const TypeSize`. Everything still seems to work (make check-all, plus an LNT run).
> > 
> > I don't think the `std::tie` comparisons are strictly equivalent to what was there before -- if `A.getScalarSizeInBits()` is greater than `B.getScalarSizeInBits()` in `LT` then I think the std::tie comparison would return false immediately instead of progressing to the second comparison as the previous code would.
> ...actually, I think it works out the same anyway, since you would only get a `true` result from `LT` in the second case if the scalar sizes were equal.
Ok, this looks a bit better, but I'm still not happy about the state of this code. I'm not going to hold your patch hostage because of this, since it was a problem of the existing code, but LE is a really really misleading name for this predicate. It suggests that it's defining a partial order, which it isn't. If A is a vector and B is a scalar (or the other way around), we get false for both A <= B and B <= A. That never happens for a mathematical A <= B (since that implies that B < A, which means B <= A should return true). OTOH, fixing it to be a proper partial order would make us remove too much. I think the best solution here would be to use a different name for the predicate - SameKindLE maybe? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66871