This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, i386, AVX] Fix typo in i386.c/standard_sse_constant_opcode ().
- From: Uros Bizjak <ubizjak at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Kirill Yukhin <kirill dot yukhin at gmail dot com>, Richard Biener <rguenther at suse dot de>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 23 Mar 2016 14:24:21 +0100
- Subject: Re: [PATCH, i386, AVX] Fix typo in i386.c/standard_sse_constant_opcode ().
- Authentication-results: sourceware.org; auth=none
- References: <20160323112150 dot GA59478 at msticlxl57 dot ims dot intel dot com> <CAFULd4YL0kL4AXeMqmzt9cqrcV7YsAHmY1MLgqjQ0VTN27=xwA at mail dot gmail dot com> <20160323125458 dot GF59478 at msticlxl57 dot ims dot intel dot com> <20160323130317 dot GZ3017 at tucnak dot redhat dot com>
On Wed, Mar 23, 2016 at 2:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 23, 2016 at 03:55:01PM +0300, Kirill Yukhin wrote:
>> > No, the change is OK (standard_sse_constant_p does all the checks),
>> > but nowadays we could write this part as:
>> >
>> > --cut here--
>> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> > index 1639704..59154c3 100644
>> > --- a/gcc/config/i386/i386.c
>> > +++ b/gcc/config/i386/i386.c
>> > @@ -10859,10 +10859,7 @@ standard_sse_constant_opcode (rtx_insn *insn, rtx x)
>> > || get_attr_mode (insn) == MODE_V8DF
>> > || get_attr_mode (insn) == MODE_V16SF)
>> > return "vpternlogd\t{$0xFF, %g0, %g0, %g0|%g0, %g0, %g0, 0xFF}";
>> > - if (TARGET_AVX)
>> > - return "vpcmpeqd\t%0, %0, %0";
>> > - else
>> > - return "pcmpeqd\t%0, %0";
>> > + return "%vpcmpeqd\t%0, %d0";
>> >
>> > default:
>> > break;
>> > -- cut here--
>> This looks much better to me.
>> Going to bootstrap/regtest.
>>
>> Is it ok to check into main trunk?
>
> But what is the advantage of doing that? You trade one simple conditional
> for lots more processing at the insn emit time (parse %v, handle if
> (TARGET_AVX) there, parse 'd' modifier, handle it if (TARGET_AVX)
> conditionally). One thing is in pattern, where it increases readability,
> but in a routine which handles several different conditions anyway?
There are several instances of the same construct in this function, so
I think this change follows the established pattern. And I would do it
in the same way in the original patch, if %d was available back
then...
So, Kirill, please go ahead with the patch (after bootstrap/regtest cycle).
Thanks,
Uros.