Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)

[Hongtao Liu <crazylht at gmail dot com>]

On Fri, May 10, 2019 at 3:55 AM Jeff Law <law@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.lu@intel.com>
> > +         Hongtao Liu  <hongtao.liu@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  <segher@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