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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1340-1343
+  if (II.isTailCall())
+    INTR_U->setTailCallKind(II.getTailCallKind());
+  if (II.isStrictFP())
+    INTR_U->addFnAttr(Attribute::StrictFP);
----------------
Having to manually choose which flags and attribute to copy seems fragile.  The current set doesn't include things like fast math flags (as can be seen from `sve-intrinsics-combine-to-u-forms.ll: replace_fabd_intrinsic_half`) and debug metadata.

This instcombine is essentially changing the name of the function called with everything else expected to remain as was so I think you should be able to use `II.setCalledFunction()`, which takes the new function declaration but will leave everything else (operands, flags, attributes...) as they are.

NOTE: This means you're not creating a new call node thus removes the need to call `replaceInstUsesWith()`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1718
+  case Intrinsic::aarch64_sve_fadd: {
+    IRBuilderBase::FastMathFlagGuard FMFGuard(IC.Builder);
+    IC.Builder.setFastMathFlags(II.getFastMathFlags());
----------------
I think my `setCalledFunction()` suggestion hopefully means this will no longer be required.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1720-1722
+    if (auto FADD_U =
+            instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fadd_u))
+      return FADD_U;
----------------
For this and other cases can you try to push the logic into the main function of the operation.  So in this case it would be better to have all the add related combines within `instCombineSVEVectorAdd`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005