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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Regarding the change to return const, I'm not convinced that's a good idea (we actually have a clang-tidy check <https://clang.llvm.org/extra/clang-tidy/checks/readability-const-return-type.html> that warns about that). I think it would be better to either name those temporaries or use std::make_tuple instead of std::tie (whichever you prefer).



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13979
+    Offset = ((int64_t)STMemType.getStoreSizeInBits() -
+              (int64_t)LDMemType.getStoreSizeInBits()) / 8 - Offset;
 
----------------
Could you be a bit more specific about why you need to cast to signed? It was unsigned in the original code too.


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:515
+    return std::tie(A.getScalarSizeInBits(), A.getSizeInBits()) <
+           std::tie(B.getScalarSizeInBits(), B.getSizeInBits());
   };
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66871