On Fri, May 10, 2019 at 3:55 AM Jeff Law <l...@redhat.com> wrote:
>
> On 5/6/19 11:38 PM, Hongtao Liu wrote:
> > Hi Uros and GCC:
> > This patch is to fix ix86_expand_sse_comi_round whose implementation
> > was not correct.
> > New implentation aligns with _mm_cmp_round_s[sd]_mask.
> >
> > Bootstrap and regression tests for x86 is fine.
> > Ok for trunk?
> >
> >
> > ChangeLog:
> > gcc/
> > * config/i386/i386-expand.c (ix86_expand_sse_comi_round):
> > Modified, original implementation isn't correct.
> >
> > gcc/testsuite
> > * gcc.target/i386/avx512f-vcomisd-2.c: New runtime tests.
> > * gcc.target/i386/avx512f-vcomisd-2.c: Likewise.
> So you'll have to bear with me, I'm not really familiar with this code,
> but in the absence of a maintainer I'll try to work through it.
>
>
> >
> > -- BR, Hongtao
> >
> >
> > 0001-Fix-ix86_expand_sse_comi_round.patch
> >
> > Index: gcc/ChangeLog
> > ===================================================================
> > --- gcc/ChangeLog (revision 270933)
> > +++ gcc/ChangeLog (working copy)
> > @@ -1,3 +1,11 @@
> > +2019-05-06 H.J. Lu <hongjiu...@intel.com>
> > + Hongtao Liu <hongtao....@intel.com>
> > +
> > + PR Target/89750
> > + PR Target/86444
> > + * config/i386/i386-expand.c (ix86_expand_sse_comi_round):
> > + Modified, original implementation isn't correct.
> > +
> > 2019-05-06 Segher Boessenkool <seg...@kernel.crashing.org>
> >
> > * config/rs6000/rs6000.md (FIRST_ALTIVEC_REGNO, LAST_ALTIVEC_REGNO)
> > Index: gcc/config/i386/i386-expand.c
> > ===================================================================
> > --- gcc/config/i386/i386-expand.c (revision 270933)
> > +++ gcc/config/i386/i386-expand.c (working copy)
> > @@ -9853,18 +9853,24 @@
> > const struct insn_data_d *insn_p = &insn_data[icode];
> > machine_mode mode0 = insn_p->operand[0].mode;
> > machine_mode mode1 = insn_p->operand[1].mode;
> > - enum rtx_code comparison = UNEQ;
> > - bool need_ucomi = false;
> >
> > /* See avxintrin.h for values. */
> > - enum rtx_code comi_comparisons[32] =
> > + static const enum rtx_code comparisons[32] =
> So I assume the comment refers to the _CMP_* #defines in avxintrin.h?
>
Yes.
>
> > {
> > - UNEQ, GT, GE, UNORDERED, LTGT, UNLE, UNLT, ORDERED, UNEQ, UNLT,
> > - UNLE, LT, LTGT, GE, GT, LT, UNEQ, GT, GE, UNORDERED, LTGT, UNLE,
> > - UNLT, ORDERED, UNEQ, UNLT, UNLE, LT, LTGT, GE, GT, LT
> > + EQ, LT, LE, UNORDERED, NE, UNGE, UNGT, ORDERED,
> > + EQ, UNLT, UNLE, UNORDERED, LTGT, GE, GT, ORDERED,
> > + EQ, LT, LE, UNORDERED, NE, UNGE, UNGT, ORDERED,
> > + EQ, UNLT, UNLE, UNORDERED, LTGT, GE, GT, ORDERED
> > };
>
> For CMP_EQ_UQ aren't we looking for an unordered comparison, so UNEQ
> seems right, but you're using EQ. Can you double-check this? If it's
> wrong, then please make sure we cover this case with a test.
>
Avx512f-vcomis[sd]-2.c covers all 32 compare predicates.
UNEQ and EQ behave differently when either operand is NAN, besides
they're the same.
Since NAN operands are handled separtely, so EQ/UNEQ makes no
difference, That why this passes cover tests.
I'll correct it.
>
>
>
> > @@ -9932,11 +10021,37 @@
> > }
> >
> > emit_insn (pat);
> > +
> > + /* XXX: Set CCFPmode and check a different CCmode. Does it work
> > + correctly? */
> > + if (GET_MODE (set_dst) != mode)
> > + set_dst = gen_rtx_REG (mode, REGNO (set_dst));
> This looks worrisome, even without the cryptic comment. I don't think
> you can just blindly change the mode like that. Unless you happen to
> know that the only things you test in the new mode were set in precisely
> the same way as the old mode.
>
Modified as:
+ /* NB: Set CCFPmode and check a different CCmode. */
+ if (GET_MODE (set_dst) != mode)
+ set_dst = gen_rtx_REG (mode, FLAGS_REG);
> Jeff
--
BR,
Hongtao
