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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked an inline comment as done.
paulwalker-arm added inline 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");
----------------
david-arm wrote:
> 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?
I've gone with option 2 as we will want this when removing other instances where we're relying on the implicit cast.


================
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()) {
----------------
david-arm wrote:
> 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?
I guess it depends on how strongly you feel about this.  It's the "almost" part that made me think commonising the code would make it less readable.  The code itself is likely to shrink as well because I've got other patches in the works that will remove the need to modify IndexType so we're only talking about the couple of lines to create the shift and constant 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123670