[PATCH] D73021: [ARM] Basic gather scatter cost model

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:901
+
+  // And (aligned) i32 gather will not need to be vectorized.
+  if (EltSize == 32)
----------------
anwel wrote:
> nit: An ?
Ah, I moved some code around and it no longer made sense.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:916
+    unsigned Scale = GEP->getPointerOperandType()
+                         ->getPointerElementType()
+                         ->getPrimitiveSizeInBits();
----------------
SjoerdMeijer wrote:
> nit: formatting?
This is apparently how clang-format wants this. But I think this line was wrong anyway. I've re-done it using DL.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:922-924
+    if (auto ZExt = dyn_cast<ZExtInst>(GEP->getOperand(1)))
+      if (ZExt->getOperand(0)->getType()->getScalarSizeInBits() <= EltSize)
+        return VectorCost;
----------------
anwel wrote:
> The gather/scatter pass can also handle a sext here, in the manner that it will just leave it the way it is and use the result if it's of the correct type. That increases `VectorCost` by the cost of the sext, but doesn't necessarily mean that building a gather is more expensive than expanding.
We will only get to this point if we are dealing with either an i16 or i8 gather, in which case we need the address to be zext to be folded into the instruction otherwise we need to expand.

For i32's, I think we can always produce something. So up above there's a check that if EltSize == 32, we return VectorCost.

It is true that the zext here will still show up as a cost where it shouldn't, the instruction will fold it in. That is something that we can fix later I think, maybe at the same time that we improve the sext/zext/trunc handling.


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

https://reviews.llvm.org/D73021