[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:1340-1343
+  if (II.isTailCall())
+    INTR_U->setTailCallKind(II.getTailCallKind());
+  if (II.isStrictFP())
+    INTR_U->addFnAttr(Attribute::StrictFP);
----------------
paulwalker-arm wrote:
> 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()`.
Fixed. And the test corrected.


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


================
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;
----------------
paulwalker-arm wrote:
> 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`.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005