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

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13979
+    Offset = ((int64_t)STMemType.getStoreSizeInBits() -
+              (int64_t)LDMemType.getStoreSizeInBits()) / 8 - Offset;
 
----------------
rovka wrote:
> Could you be a bit more specific about why you need to cast to signed? It was unsigned in the original code too.
This code was buggy before, but didn't crash as `getStoreSizeInBits` returned a 32 bit `unsigned`, vs. the `uint64_t` we now return as an implicit cast.

For the failing case, LDMemType's size was larger than STMemType's size, so wrapped to a very large uint64_t value. That was then divided by 8 (which unset the top bit due to being an unsigned divide), implicitly cast to an int64_t so that Offset (which was zero) could be subtracted.

The next bit of code multiplies Offset by 8, which wraps for a signed value (which UBSan caught).

It didn't happen for the 32 bit result since the top bits were clear at the time of the implicit cast to int64_t.

I suspect the bug would have been found quickly once cases with a non-zero Offset were implemented, but the code below currently bails out in that case.


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:515
+    return std::tie(A.getScalarSizeInBits(), A.getSizeInBits()) <
+           std::tie(B.getScalarSizeInBits(), B.getSizeInBits());
   };
----------------
rovka wrote:
> I was actually thinking of including isScalableVector and isVector below in the tie (since bool is an integral type and compares the way you'd expect).
Yeah, I thought of that too, but ran into problems when trying to make it const (maybe because `bool` is a primitive?).

I'll try with `make_tuple` and see what happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66871