Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Sat, Jun 1, 2019 at 6:08 AM Jeff Law wrote:
>
> On 5/30/19 2:53 AM, Hongtao Liu wrote:
> > On Thu, May 30, 2019 at 3:23 AM Jeff Law wrote:
> >> On 5/9/19 10:54 PM, Hongtao Liu wrote:
> >>> On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
> > + Hongtao Liu
> > +
> > + 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
> >
> > * 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 = _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.
> >> Ah. Thanks. FWIW my approach was to walk through the _CMP_* defines in
> >> avxintrin.h and map that to what I thought the comparison ought to be.
> >> Then I reviewed my result against your patch. I got a couple wrong, but
> >> could easily see my mistake. The only one I couldn't reconcile was the
> >> CMP_EQ_UQ. Knowing the NaNs are handled separately makes it clear.
> >> Thanks gain.
> >>
> >>
> >>
>
>
> > @@ -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);
> >> That might actually be worse. The mode carries semantic information
> >> about where to find the various condition codes within the flags
> >> register and which
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On 5/30/19 2:53 AM, Hongtao Liu wrote:
> On Thu, May 30, 2019 at 3:23 AM Jeff Law wrote:
>> On 5/9/19 10:54 PM, Hongtao Liu wrote:
>>> On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
> + Hongtao Liu
> +
> + 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
>
> * 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 = _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.
>> Ah. Thanks. FWIW my approach was to walk through the _CMP_* defines in
>> avxintrin.h and map that to what I thought the comparison ought to be.
>> Then I reviewed my result against your patch. I got a couple wrong, but
>> could easily see my mistake. The only one I couldn't reconcile was the
>> CMP_EQ_UQ. Knowing the NaNs are handled separately makes it clear.
>> Thanks gain.
>>
>>
>>
> @@ -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);
>> That might actually be worse. The mode carries semantic information
>> about where to find the various condition codes within the flags
>> register and which condition codes are valid. The register number
>> determines which (of possibly many) flags registers we are querying.
>>
>> Thus if the mode of SET_DEST is not the same as MODE, then there is a
>> mismatch between the point where the condition codes were set and where
>> we want to use
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Thu, May 30, 2019 at 3:23 AM Jeff Law wrote:
>
> On 5/9/19 10:54 PM, Hongtao Liu wrote:
> > On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
> >>> + Hongtao Liu
> >>> +
> >>> + 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
> >>>
> >>> * 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 = _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.
> Ah. Thanks. FWIW my approach was to walk through the _CMP_* defines in
> avxintrin.h and map that to what I thought the comparison ought to be.
> Then I reviewed my result against your patch. I got a couple wrong, but
> could easily see my mistake. The only one I couldn't reconcile was the
> CMP_EQ_UQ. Knowing the NaNs are handled separately makes it clear.
> Thanks gain.
>
>
>
> >>
> >>
> >>
> >>> @@ -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);
> That might actually be worse. The mode carries semantic information
> about where to find the various condition codes within the flags
> register and which condition codes are valid. The register number
> determines which (of possibly many) flags registers we are querying.
>
> Thus if the mode of SET_DEST is not the same as MODE, then there is a
> mismatch between the point where the condition codes were set and where
> we want to use them.
>
> That can only be safe is
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On 5/9/19 10:54 PM, Hongtao Liu wrote:
> On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
>>> + Hongtao Liu
>>> +
>>> + 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
>>>
>>> * 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 = _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.
Ah. Thanks. FWIW my approach was to walk through the _CMP_* defines in
avxintrin.h and map that to what I thought the comparison ought to be.
Then I reviewed my result against your patch. I got a couple wrong, but
could easily see my mistake. The only one I couldn't reconcile was the
CMP_EQ_UQ. Knowing the NaNs are handled separately makes it clear.
Thanks gain.
>>
>>
>>
>>> @@ -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);
That might actually be worse. The mode carries semantic information
about where to find the various condition codes within the flags
register and which condition codes are valid. The register number
determines which (of possibly many) flags registers we are querying.
Thus if the mode of SET_DEST is not the same as MODE, then there is a
mismatch between the point where the condition codes were set and where
we want to use them.
That can only be safe is the condition codes set in the new mode are a
strict subset of the condition codes set in the old mode and they're
found in the same place within the same flags register.
Maybe a simple example would help. Consider a fairly standard target
with arithmetic insns that set ZNV and logicals that
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Fri, May 10, 2019 at 12:54 PM Hongtao Liu wrote:
>
> On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
> > > + Hongtao Liu
> > > +
> > > + 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
> > >
> > > * 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 = _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
Update patch.
--
BR,
Hongtao
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 271051)
+++ gcc/ChangeLog (working copy)
@@ -316,6 +316,14 @@
* tree-ssa-phiopt.c (two_value_replacement): Fix a typo in parameter
detection.
+2019-05-06 H.J. Lu
+ Hongtao Liu
+
+ 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
* config/rs6000/rs6000.md (FIRST_ALTIVEC_REGNO, LAST_ALTIVEC_REGNO)
Index: gcc/config/i386/i386-expand.c
===
--- gcc/config/i386/i386-expand.c (revision 271051)
+++ gcc/config/i386/i386-expand.c (working copy)
@@ -9877,18 +9877,24 @@
const struct insn_data_d *insn_p = _data[icode];
machine_mode mode0 =
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Fri, May 10, 2019 at 3:55 AM Jeff Law 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
> > + Hongtao Liu
> > +
> > + 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
> >
> > * 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 = _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
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
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
> + Hongtao Liu
> +
> + 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
>
> * 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 = _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?
> {
> - 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.
> @@ -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.
Jeff
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Wed, May 8, 2019 at 6:28 AM Hongtao Liu wrote: > > Any other comments, i'll merge to trunk? You are not allowed to merge patches without explicit approval from the relevant maintainer. Please see [1]. However, there will be a problem with the approval of your patch. AVX512 stuff has its own maintainer, which unfortunatelly went missing. I don't plan to be a backup maintainer of AVX512 stuff, so the patch may be left unreviewed for a long time. AVX512 stuff is very low (or better, nowhere) on my priority list, so there is little point on pinging me for a review of patches involving AVX512 functionality. [1] https://gcc.gnu.org/svnwrite.html#policies Uros. > > On Tue, May 7, 2019 at 3:31 PM Hongtao Liu wrote: > > > > On Tue, May 7, 2019 at 3:03 PM Jakub Jelinek wrote: > > > > > > On Tue, May 07, 2019 at 01:38:49PM +0800, Hongtao Liu wrote: > > > > +2019-05-06 H.J. Lu > > > > + Hongtao Liu > > > > + > > > > + PR Target/89750 > > > > + PR Target/86444 > > > > > > target, not Target. Various people handle these in various scripts, > > > so it is better to use consistency and exact spelling of the categories. > > > > > > Jakub > > > > Ok, Thank you for your reminding. > > > > -- > > BR, > > Hongtao > > > > -- > BR, > Hongtao
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
Any other comments, i'll merge to trunk? On Tue, May 7, 2019 at 3:31 PM Hongtao Liu wrote: > > On Tue, May 7, 2019 at 3:03 PM Jakub Jelinek wrote: > > > > On Tue, May 07, 2019 at 01:38:49PM +0800, Hongtao Liu wrote: > > > +2019-05-06 H.J. Lu > > > + Hongtao Liu > > > + > > > + PR Target/89750 > > > + PR Target/86444 > > > > target, not Target. Various people handle these in various scripts, > > so it is better to use consistency and exact spelling of the categories. > > > > Jakub > > Ok, Thank you for your reminding. > > -- > BR, > Hongtao -- BR, Hongtao
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Tue, May 7, 2019 at 3:03 PM Jakub Jelinek wrote: > > On Tue, May 07, 2019 at 01:38:49PM +0800, Hongtao Liu wrote: > > +2019-05-06 H.J. Lu > > + Hongtao Liu > > + > > + PR Target/89750 > > + PR Target/86444 > > target, not Target. Various people handle these in various scripts, > so it is better to use consistency and exact spelling of the categories. > > Jakub Ok, Thank you for your reminding. -- BR, Hongtao
Re: [Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
On Tue, May 07, 2019 at 01:38:49PM +0800, Hongtao Liu wrote: > +2019-05-06 H.J. Lu > + Hongtao Liu > + > + PR Target/89750 > + PR Target/86444 target, not Target. Various people handle these in various scripts, so it is better to use consistency and exact spelling of the categories. Jakub
[Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)
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.
--
BR,
Hongtao
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 270933)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,11 @@
+2019-05-06 H.J. Lu
+ Hongtao Liu
+
+ 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
* 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 = _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] =
{
- 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
};
- bool need_ucomi_values[32] =
+ static const bool ordereds[32] =
{
+ true, true, true, false, false, false, false, true,
+ false, false, false, true, true, true, true, false,
+ true, true, true, false, false, false, false, true,
+ false, false, false, true, true, true, true, false
+};
+ static const bool non_signalings[32] =
+{
true, false, false, true, true, false, false, true,
true, false, false, true, true, false, false, true,
false, true, true, false, false, true, true, false,
@@ -9888,16 +9894,95 @@
return const0_rtx;
}
- comparison = comi_comparisons[INTVAL (op2)];
- need_ucomi = need_ucomi_values[INTVAL (op2)];
-
if (VECTOR_MODE_P (mode0))
op0 = safe_vector_operand (op0, mode0);
if (VECTOR_MODE_P (mode1))
op1 = safe_vector_operand (op1, mode1);
+ enum rtx_code comparison = comparisons[INTVAL (op2)];
+ bool ordered = ordereds[INTVAL (op2)];
+ bool non_signaling = non_signalings[INTVAL (op2)];
+ rtx const_val = const0_rtx;
+
+ bool check_unordered = false;
+ machine_mode mode = CCFPmode;
+ switch (comparison)
+{
+case ORDERED:
+ if (!ordered)
+ {
+ /* NB: Use CCSmode/NE for _CMP_TRUE_UQ/_CMP_TRUE_US. */
+ if (!non_signaling)
+ ordered = true;
+ mode = CCSmode;
+ }
+ else
+ {
+ /* NB: Use CCPmode/NE for _CMP_ORD_Q/_CMP_ORD_S. */
+ if (non_signaling)
+ ordered = false;
+ mode = CCPmode;
+ }
+ comparison = NE;
+ break;
+case UNORDERED:
+ if (ordered)
+ {
+ /* NB: Use CCSmode/EQ for _CMP_FALSE_OQ/_CMP_FALSE_OS. */
+ if (non_signaling)
+ ordered = false;
+ mode = CCSmode;
+ }
+ else
+ {
+ /* NB: Use CCPmode/NE for _CMP_UNORD_Q/_CMP_UNORD_S. */
+ if (!non_signaling)
+ ordered = true;
+ mode = CCPmode;
+ }
+ comparison = EQ;
+ break;
+
+case LE: /* -> GE */
+case LT: /* -> GT */
+case UNGE: /* -> UNLE */
+case UNGT: /* -> UNLT */
+ std::swap (op0, op1);
+ comparison = swap_condition (comparison);
+ /* FALLTHRU */
+case GT:
+case GE:
+case UNEQ:
+case UNLT:
+case UNLE:
+case LTGT:
+ /* These are supported by CCFPmode. NB: Use ordered/signaling
+ COMI or unordered/non-signaling UCOMI. Both set ZF, PF, CF
+ with NAN operands. */
+ if (ordered == non_signaling)
+ ordered = !ordered;
+ break;
+case EQ:
+ if (ordered)
+ check_unordered = true;
+ /* NB: COMI/UCOMI will set ZF with NAN operands. Use CCmode for
+ _CMP_EQ_OQ/_CMP_EQ_OS/_CMP_EQ_UQ/_CMP_EQ_US. */
+ mode = CCmode;
+ break;
+case NE:
+ gcc_assert (!ordered);
+ check_unordered = true;
+ /* NB: COMI/UCOMI will set ZF with NAN operands. Use CCmode for
+ _CMP_NEQ_UQ/_CMP_NEQ_US. */
+ mode = CCmode;
+ const_val = const1_rtx;
+ break;
+default:
+ gcc_unreachable ();
+}
+
target = gen_reg_rtx (SImode);
- emit_move_insn (target,
