On Sat, Oct 12, 2019 at 4:15 PM Jakub Jelinek <ja...@redhat.com> wrote:
>
> 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
Thanks for your review, that helps a lot.
--
BR,
Hongtao
