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

[Graham Hunter via Phabricator via llvm-commits]

huntergr marked 3 inline comments as done.
huntergr added a comment.

I also changed the SelectionDAGBuilder code for masked loads/stores/gathers/scatters to use the known min size when creating a MachineMemoryOperand, since MMO isn't aware of scalable types yet. I've left TODOs as reminders.



================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:506
   auto LT = [](MVT A, MVT B) -> bool {
+    // Always treat non-scalable MVTs as smaller than scalable MVTs for the
+    // purposes of ordering.
----------------
sdesmalen wrote:
> Is this a good argument to add an interface for `isKnownGreater` and `isKnownGreaterOrEqual` (vis-a-vis `isKnownSmaller`/`IsKnownSmallerOrEqual`) to TypeSize as suggested in https://reviews.llvm.org/D53137#1621351 ?
After thinking about it for a bit, I don't think so.

`isKnownSmaller` would return false for `v8f32` vs. `nxv4f32`, and we would then proceed with a direct comparison of scalable and non-scalable type sizes, which will assert. Using that as the only comparison would also ignore the different ordering in this function, where a v4f32 would be considered less than an i64 since the scalar size is compared first.

It's possible that a different canonical ordering would work as well, but I'm not sure how many places in CodeGenDAGPatterns assume things about the order. Using the `std::tie` comparisons changes things slightly as it is, as noted below.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66871