[PATCH] D123670: [SVE] Add support for non-element-type sized scaling when lowering MGATHER/MSCATTER.

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This looks like a nice couple of fixes @paulwalker-arm, thanks! I had a couple of minor comments ...



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4675
+  uint64_t ScaleVal = cast<ConstantSDNode>(Scale)->getZExtValue();
+  if (IsScaled && ScaleVal != MemVT.getScalarType().getStoreSize()) {
+    assert(isPowerOf2_64(ScaleVal) && "Expecting power-of-two types");
----------------
Hi @paulwalker-arm, you're introducing an implicit TypeSize->uint64_t cast here. Could you either:

1. Change this to MemVT.getScalarType().getStoreSize().getFixedSize(), or alternatively ...
2. Add a new function EVT called getScalarStoreSize(), similar to getScalarSizeInBits(), if you think that's useful?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4791
+  // must be calculated before hand.
+  uint64_t ScaleVal = cast<ConstantSDNode>(Scale)->getZExtValue();
+  if (IsScaled && ScaleVal != MemVT.getScalarType().getStoreSize()) {
----------------
This code looks almost identical to the code in LowerMGATHER, except one creates a masked gather at the end and the other creates a masked scatter. Is it worth commonising at least part of this code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123670