[PATCH] D109417: Cost model for VPMemory operations on PowerPC.

[Roland Froese via Phabricator via llvm-commits]

RolandF requested changes to this revision.
RolandF added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1362
+  // VSX masks have lanes per bit, but the predication is per halfword.
+  unsigned NumElems = SrcVTy->getNumElements();
+  auto *SrcScalarTy = SrcVTy->getScalarType();
----------------
These variables are only used in the if block and should be declared there.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1363
+  unsigned NumElems = SrcVTy->getNumElements();
+  auto *SrcScalarTy = SrcVTy->getScalarType();
+  auto *MaskI8Ty = Type::getInt8Ty(SrcVTy->getContext());
----------------
This variable is only used once - the expression can be used directly instead.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1367
+
+  if (!getVPLegalizationStrategy(*dyn_cast<VPIntrinsic>(I)).shouldDoNothing()) {
+    // Currently we can only lower intrinsics with evl but no mask, on Power
----------------
The Instruction pointer I is optional but is dereferenced unconditionally.  The is likely to segfault.  There is a dyn_cast but the result is not checked for nullptr so there may also be a segfault if I is something else.  If a valid vp intrinsic pointer is expected for this target implementation then there should be an assert to check the dyn_cast result.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1367
+
+  if (!getVPLegalizationStrategy(*dyn_cast<VPIntrinsic>(I)).shouldDoNothing()) {
+    // Currently we can only lower intrinsics with evl but no mask, on Power
----------------
RolandF wrote:
> The Instruction pointer I is optional but is dereferenced unconditionally.  The is likely to segfault.  There is a dyn_cast but the result is not checked for nullptr so there may also be a segfault if I is something else.  If a valid vp intrinsic pointer is expected for this target implementation then there should be an assert to check the dyn_cast result.
Having the entire middle of the function occupied by a path that one probably never wants to execute is slightly painful.  I would suggest handling the shouldDoNothing path first instead.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1402
+
+  if (Alignment >= 16)
+    // If the op is guaranteed to be aligned to 128 bytes,
----------------
If this is made an or condition with not pwr9 then the pwr10 case can return here so there aren't two return LT.first lines.


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:1405
+    // then VSX masked memops cost the same as unmasked memops.
+    return LT.first;
+
----------------
Missing vectorCostAdjustment to account for P9.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109417