[PATCH] D85138: [ARM][MVE] Enable tail predication for loops containing MVE gather/scatters

[Anna Welker via Phabricator via llvm-commits]

anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.h:860
+  unsigned IntrinsicID = IntInst->getIntrinsicID();
+  return (IntrinsicID == Intrinsic::masked_scatter ||
           IntrinsicID == Intrinsic::arm_mve_vstr_scatter_base ||
----------------
samparker wrote:
> Like Dave also said, do we really need the generic IR opcodes in these helpers?
In the way this function is intended to work, no we don't. Checking for the generic IR opcodes in the case the function is used for here might seem a bit overkill, but not harmful to the intentions of the check (please do correct me if I'm mistaken).
It is however only a re-use of the function, which was originally intended for a use in MVEGatherScatterLowering, for checking whether all users of a getelementptr instruction are gathers or scatters. Knowing this allows for certain transformations based on assumptions that can be made about such addresses. In that case it is important to include the generic ones, because at the time the check is made not all gathers and scatters have been transformed yet.
I was trying to avoid code duplication with this, but if your advice is that it would be better to separate the functionality more clearly, I'm happy to introduce an `isMVEGather` etc


================
Comment at: llvm/lib/Target/ARM/MVETailPredication.cpp:237-251
+  unsigned ID = I->getIntrinsicID();
+  FixedVectorType *VecTy;
+  if (ID == Intrinsic::masked_load || ID == Intrinsic::masked_store) {
+    unsigned TypeOp = (ID == Intrinsic::masked_load) ? 0 : 1;
+    auto *PtrTy = cast<PointerType>(I->getOperand(TypeOp)->getType());
+    VecTy = dyn_cast<FixedVectorType>(PtrTy->getElementType());
+  } else if (ID == Intrinsic::masked_scatter ||
----------------
dmgreen wrote:
> Can this entire function use:
>  - The return type of a load/gather
>  - The value type of a store/scatter
> ?
Yes, if I see it correctly, it can. I changed it to do that.


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

https://reviews.llvm.org/D85138