[PATCH] D94230: [AArch64][SVE] Add SVE IR pass to coalesce ptrue instrinsic calls

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Could you update the commit message as I think it's now out of date? For example, "This commit introduces a new pass ..."



================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:151
+        // Would some lanes become zeroed by the conversion?
+        if (IntrUserVTy->getElementCount().getKnownMinValue() >
+            PTrueVTy->getElementCount().getKnownMinValue())
----------------
Again, this check is only valid if both types are fixed width or both types are scalable.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:174
+        auto *PTrue2VTy = cast<VectorType>(PTrue2->getType());
+        return PTrue1VTy->getElementCount().getKnownMinValue() <
+               PTrue2VTy->getElementCount().getKnownMinValue();
----------------
Do we ever expect fixed width types here? If not, I wonder if you should cast to ScalableVectorType instead. The reason I mention this is that your comparison below only works when all are fixed width or all are scalable. 


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:202
+    Builder.SetInsertPoint(&BB, ++ConvertToSVBool->getIterator());
+    auto *ConvertFromSVBool =
+        Builder.CreateIntrinsic(Intrinsic::aarch64_sve_convert_from_svbool,
----------------
What if there are multiple ptrues with the same type as the most encompassing? We'll be needlessly creating convert_from_svbool intrinsics here I think? I think in this case we can just call replaceAllUsesWith(MostEncompassingPTrueVTy)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94230