[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:1715-1774
+  case Intrinsic::aarch64_sve_fabd:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fabd_u);
+  case Intrinsic::aarch64_sve_fdiv:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fdiv_u);
+  case Intrinsic::aarch64_sve_fmax:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fmax_u);
+  case Intrinsic::aarch64_sve_fmaxnm:
----------------
mgabka wrote:
> would be good if the order of intrinsics here aligned with the order in the test file so it would be easier to check that we have full coverage
I think they are. But I could make a mistake, I'll check.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-m-to-x.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
----------------
mgabka wrote:
> I am not a big fan of the name of this file, it suggests that there should be intrinsics with _x or _m suffix being used but there no such ones.
> 
> I think it would also be good to sort the test in alphabetical order by the name of the intrinsic being tested, that would make navigation through this long file easier.
_x and _m suffixes were inspired by [[ https://developer.arm.com/architectures/instruction-sets/intrinsics | acle docs ]]. I happily take suggestions to rename the file.

Shall the alphabetical sorting be for the whole file or within the sections, i.e. within Float arithmetics, Integer arithmetics, Shifts and Logical operations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005