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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
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:
> 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.
Actually the `getVPLegalizationStrategy` doesn't seem like the right interface to use (because we may not have the VP intrinsics available at the time we do cost-analysis). What we really need is something that can tell us if the opcode/type combination is considered legal on a target (ie PPC). I think I asked this question before, and someone told me to use `hasActiveVectorLength()`. However, I think we need to change that interface to take in more pieces of info. I'm going to look into that next.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109417