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

[mgabka via Phabricator via llvm-commits]

mgabka added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1286
+
+  Module *M = II.getParent()->getParent()->getParent();
+  Function *NewDecl = Intrinsic::getDeclaration(M, IID, {II.getType()});
----------------
I think the preference is to use 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);
----------------
I think the preference is to use 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 {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005