Hi!
> gcc/
> * config/i386/avx512fintrin.h (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss, _mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_mask_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): New intrinsics.
> (_mm_roundscale_ss, _mm_roundscale_round_ss): Fix.
"Fix." doesn't describe the change you've done. So I think it should be
instead:
"Use __builtin_ia32_rndscales?_mask_round builtins instead of
__builtin_ia32_rndscales?_round."
> * config/i386/i386-builtin.def (__builtin_ia32_rndscaless_round,
> __builtin_ia32_rndscalesd_round): Remove.
> (__builtin_ia32_rndscalesd_mask_round,
Pasto, sd listed twice, ss not listed, change the first one to ss.
> __builtin_ia32_rndscalesd_mask_round): New intrinsics.
> * config/i386/sse.md (avx512f_rndscale<mode><round_saeonly_name>):
> Renamed to ...
> (avx512f_rndscale<mode><mask_scalar_name><round_saeonly_scalar_name>):
> ... this.
These two lines are too long. Perhaps:
* config/i386/sse.md
(avx512f_rndscale<mode><round_saeonly_name>): Renamed to ...
(avx512f_rndscale<mode><mask_scalar_name><round_saeonly_scalar_name>):
... this.
> ((match_operand:VF_128 2 "<round_saeonly_nimm_predicate>"
> "<round_saeonly_constraint>")): Changed to ...
> ((match_operand:VF_128 2 "<round_saeonly_scalar_nimm_predicate>"
> "<round_saeonly_scalar_constraint>")): ... this.
> ("vrndscale<ssescalarmodesuffix>\t{%3, <round_saeonly_op4>%2, %1,
> %0|%0, %1, %<iptr>2<round_saeonly_op4>, %3}"): Changed to ...
>
> ("vrndscale<ssescalarmodesuffix>\t{%3,<round_saeonly_scalar_mask_op4>%2, %1,
> %0<mask_scalar_operand4>|%0<mask_scalar_operand4>, %1,
> %<iptr>2<round_saeonly_scalar_mask_op4>, %3}"): ... this.
But the above is not appropriate, the ChangeLog in *.md is at the level of
define_{insn,expand,split,peephole2,insn_and_split} etc., not at the level
of individual subrtls or patterns.
So, the right thing is just to ammend the "... this.", follow it up by a
sentence what also changed. Like " Adjust and add subst attributes to make
it maskable."
>
> gcc/testsuite/
> * gcc.target/i386/avx512f-vrndscaless-1.c (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss): Test new intrinsics.
That is again not what you've changed. For tests, often the exact
spots aren't listed and one just uses *.c: Description. but if you want to
use details, you can e.g.
* gcc.target/i386/avx512f-vrndscaless-1.c: Add scan-assembler-times
directives for newly expected instructions.
(m): New variable.
(avx512f_test): Add tests for new intrinsics.
> * gcc.target/i386/avx512f-vrndscaless-2.c (_mm_mask_roundscale_ss,
> _mm_maskz_roundscale_ss, _mm_maskz_roundscale_round_ss,
> _mm_maskz_roundscale_round_ss): Test new intrinsics.
> * gcc.target/i386/avx512f-vrndscalesd-1.c (_mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): Test new intrinsics.
> * gcc.target/i386/avx512f-vrndscalesd-2.c (_mm_mask_roundscale_sd,
> _mm_maskz_roundscale_sd, _mm_maskz_roundscale_round_sd,
> _mm_maskz_roundscale_round_sd): Test new intrinsics.
Likewise.
> * gcc.target/i386/avx-1.c (__builtin_ia32_rndscalefss_round,
> __builtin_ia32_rndscalefsd_round): Remove builtin.
> (__builtin_ia32_rndscalefss_mask_round,
> __builtin_ia32_rndscalefsd_mask_round): Test new builtin.
That is not what you've changed. You are there not Removing a builtin
and testing a new builtin, but removing a macro and defining a new macro.
So I think
: Remove.
: Define.
is more appropriate.
> +#define _mm_roundscale_round_ss(A, B, I, R)
> \
> + ((__m128) __builtin_ia32_rndscaless_mask_round ((__v4sf)(__m128)(A),
> \
> + (__v4sf)(__m128)(B), \
> + (int)(I), \
> + (__v4sf)_mm_setzero_ps(),\
There should be a space after _mm_setzero_p[sd] .
I know the formatting of many macros is just wrong, but no need to add more
of that.
Ok for trunk with those nits fixed.
In fact, it would be probably cleaner to:
((__m128) \
__builtin_ia32_rndscaless_mask_round ((__v4sf) (__m128) (A), \
(__v4sf) (__m128) (B), \
(int) (I), \
(__v4sf) _mm_setzero_ps (), \
(__mmask8) (-1), \
(int) (R))
or so, because then one has for long builtin names more space. But
I'm not asking for that to be changed.
Jakub
