[PATCH] D109416: getVPMemoryOpCost interface

[Roland Froese via Phabricator via llvm-commits]

RolandF added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1121
+  InstructionCost
+  getVPMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
+                    unsigned AddressSpace,
----------------
bmahjour wrote:
> simoll wrote:
> > RolandF wrote:
> > > RolandF wrote:
> > > > bmahjour wrote:
> > > > > RolandF wrote:
> > > > > > I prefer the downstream name getVariableLengthMemoryOpCost.  I'm not sure
> > > > > > what VP stands for here and it seems like VP is already used in the context of VPlan.  At least explain in the comment.
> > > > > VP stands for Vector Predication and is the name that the community has used for all the predicated intrinsics. See https://llvm.org/docs/LangRef.html#vector-predication-intrinsics
> > > > Well, I followed your link.  There are no "vp" memory ops.  The loads and stores are named "masked" operations.
> > > The target implementation you propose depends on having an Instruction parameter passed in.  If you pass in an Instruction you could actually figure out from the dynamic cast that this is a vp intrinsic.  The existing getMemoryOpCost allows passing in an Instruction, so it seems like you could figure this out without a new interface.
> > The LLVM LangRef wording for vp.load/store/gather/scatter are missing. The intrinsics themselves are upstream though.
> There is precedence for this, for example we have separate interfaces for `getMaskedMemoryOpCost` and `getGatherScatterOpCost`, so we try to follow that in this patch. 
> I think the instruction being passed to `getMemoryOpCost` is meant to be optional, and it may not even be a vp intrinsic call. For example we could be querying the cost of turning a scalar load into a vector predicated load intrinsic, in which case the instruction passed to `getMemoryOpCost` would be a scalar load so we cannot do a dyn_cast on it to figure out if we want a vp intrinsic cost or otherwise.
Now that the instruction pointer is truly optional, having a separate call actually provides value and I like this patch a lot better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109416