[PATCH] D152005: [SVE ACLE] Implement IR combines to convert intrinsics used for _m C/C++ builtins

[Jolanta Jensen via Phabricator via llvm-commits]

jolanta.jensen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1278
 
+static std::optional<Instruction *> instCombineSVEAllActive(InstCombiner &IC,
+                                                            IntrinsicInst &II,
----------------
mgabka wrote:
> looks like it is not used anymore so can be removed
Removed.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1286
+
+  Module *M = II.getParent()->getParent()->getParent();
+  Function *NewDecl = Intrinsic::getDeclaration(M, IID, {II.getType()});
----------------
mgabka wrote:
> I think the preference is to use auto
Changed to auto.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1287
+  Module *M = II.getParent()->getParent()->getParent();
+  Function *NewDecl = Intrinsic::getDeclaration(M, IID, {II.getType()});
+  II.setCalledFunction(NewDecl);
----------------
mgabka wrote:
> I think the preference is to use auto
Changed to auto.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-combine-to-u-forms.ll:66
+
+declare <vscale x 8 x half> @llvm.aarch64.sve.fadd.nxv8f16(<vscale x 8 x i1>, <vscale x 8 x half>, <vscale x 8 x half>)
+define <vscale x 8 x half> @replace_fadd_intrinsic_half(<vscale x 8 x half> %a, <vscale x 8 x half> %b) #0 {
----------------
paulwalker-arm wrote:
> mgabka wrote:
> > I have a question here, would it make more sense to have a separate test file for the cases where intrinsic is combined to a LLVM instructions?
> > or maybe worth to add a comment to clarify why in this case we do not expect _u intrinsic?
> These tests do exercise the new code so I think they belong in this file. Adding a comment to acknowledge the perhaps unexpected output sounds reasonable.
Added comments for fadd, fmul and fsub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005