Hi Kyrill,
Thank you for the review. I have addressed all the comments here:
https://gcc.gnu.org/pipermail/gcc-patches/2024-August/658968.html
Thanks,
Saurabh
On 7/22/2024 12:57 PM, Kyrylo Tkachov wrote:
Hi Saurabh,
On 22 Jul 2024, at 13:39, saurabh....@arm.com wrote:
External email: Use caution opening links or attachments
The AArch64 FEAT_FAMINMAX extension is optional in Armv9.2 and mandatory
in Armv9.5. It introduces instructions for computing the floating point
absolute maximum and minimum of the two vectors element-wise.
This patch introduces intrinsics for AdvSIMD faminmax extension in the
form of the following builtin-functions:
* vamax_f16
* vamaxq_f16
* vamax_f32
* vamaxq_f32
* vamaxq_f64
* vamin_f16
* vaminq_f16
* vamin_f32
* vaminq_f32
* vaminq_f64
gcc/ChangeLog:
* config/aarch64/aarch64-builtins.cc
(enum aarch64_builtins): New enum values for faminmax builtins.
(aarch64_init_faminmax_builtins): New function to declare new
builtins.
(handle_arm_neon_h): Modified to call
aarch64_init_faminmax_builtins.
(aarch64_general_check_builtin_call): Modified to check whether
+faminmax flag is being used and printing error message if not used.
(aarch64_expand_builtin_faminmax): New function to emit
instructions of this extension.
(aarch64_general_expand_builtin): Modified to call
aarch64_expand_builtin_faminmax.
* config/aarch64/aarch64-option-extensions.def
(AARCH64_OPT_EXTENSION): Introduce new flag for this extension.
* config/aarch64/aarch64-simd.md
(aarch64_<faminmax><mode>): Introduce instruction pattern for this
extension.
* config/aarch64/aarch64.h
(TARGET_FAMINMAX): Introduce new flag for this extension.
* config/aarch64/iterators.md: Introduce new iterators for this
extension.
* config/arm/types.md: Introduce neon_fp_aminmax<q> attributes.
* doc/invoke.texi: Document extension in AArch64 Options.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/simd/faminmax.c: New tests for this
extension.
---
Hi,
Regression tested for aarch64-none-linux-gnu and found no regressions.
This patch is dependent on the patch series "Extend
aarch64_feature_flags to 128 bits" which is under review. This patch
should be commited only after that patch series is commited. I am
raising this patch now for early feedback.
Ok for master? I don't have commit access so can someone please commit
on my behalf?
Regards,
Saurabh
---
gcc/config/aarch64/aarch64-builtins.cc | 150 ++++++++++++++++--
.../aarch64/aarch64-option-extensions.def | 2 +
gcc/config/aarch64/aarch64-simd.md | 11 ++
gcc/config/aarch64/aarch64.h | 4 +
gcc/config/aarch64/iterators.md | 10 ++
gcc/config/arm/types.md | 6 +
gcc/doc/invoke.texi | 2 +
.../gcc.target/aarch64/simd/faminmax.c | 40 +++++
8 files changed, 216 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/faminmax.c
<0001-aarch64-Add-ACLE-intrinsics-for-AdvSIMD-faminmax.patch>
@@ -2197,15 +2269,34 @@ aarch64_general_check_builtin_call (location_t location,
vec<location_t>,
case AARCH64_WSR64:
case AARCH64_WSRF:
case AARCH64_WSRF64:
- tree addr = STRIP_NOPS (args[0]);
- if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE
- || TREE_CODE (addr) != ADDR_EXPR
- || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST)
- {
- error_at (location, "first argument to %qD must be a string literal",
- fndecl);
- return false;
- }
+ {
+ tree addr = STRIP_NOPS (args[0]);
+ if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE
+ || TREE_CODE (addr) != ADDR_EXPR
+ || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST)
+ {
+ error_at (location,
+ "first argument to %qD must be a string literal",
+ fndecl);
+ return false;
+ }
+ }
+ case AARCH64_FAMINMAX_BUILTIN_FAMAX8H:
+ case AARCH64_FAMINMAX_BUILTIN_FAMAX2S:
+ case AARCH64_FAMINMAX_BUILTIN_FAMAX4S:
+ case AARCH64_FAMINMAX_BUILTIN_FAMAX2D:
+ case AARCH64_FAMINMAX_BUILTIN_FAMIN4H:
+ case AARCH64_FAMINMAX_BUILTIN_FAMIN8H:
+ case AARCH64_FAMINMAX_BUILTIN_FAMIN2S:
+ case AARCH64_FAMINMAX_BUILTIN_FAMIN4S:
+ case AARCH64_FAMINMAX_BUILTIN_FAMIN2D:
+ {
+ if (!TARGET_FAMINMAX)
+ {
+ error_at (location, "need +faminmax flag to call %qD", fndecl);
I’d rather keep these error messages consistent around the backend.
Can we reuse report_missing_extension from aarch64-sve-builtins.cc
<http://aarch64-sve-builtins.cc/> somehow to have the “intrinsic requires +foo
extension” reporting in one place?
diff --git a/gcc/config/aarch64/aarch64-simd.md
b/gcc/config/aarch64/aarch64-simd.md
index bbeee221f37..c769a482312 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -9881,3 +9881,14 @@
"shl\\t%d0, %d1, #16"
[(set_attr "type" "neon_shift_imm")]
)
+
+;; faminmax instruction patterns
+(define_insn "aarch64_<faminmax><mode>"
+ [(set (match_operand:VHSDF 0 "register_operand" "=w")
+ (unspec:VHSDF [(match_operand:VHSDF 1 "register_operand" "w")
+ (match_operand:VHSDF 2 "register_operand" "w")]
+ FAMINMAX_UNS))]
+ "TARGET_FAMINMAX"
+ "<faminmax>\t%0.<Vtype>, %1.<Vtype>, %2.<Vtype>"
+ [(set_attr "type" "neon_fp_aminmax<q>")]
+)
Any reason why we can’t represent these using organic RTL codes for smin/smax
and abs?
It would allow the RTL optimizers to more easily see through the semantics.
Or is the NaN/Inf handling of these instructions inconsistent with the RTL
semantics?
Thanks,
Kyrill
