* PATCH to add more FALLTHRU markers
@ 2016-09-27 16:55 Marek Polacek
2016-09-27 19:00 ` Richard Sandiford
2016-09-27 19:08 ` Florian Weimer
0 siblings, 2 replies; 12+ messages in thread
From: Marek Polacek @ 2016-09-27 16:55 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Sandiford
Currently Makefile.in contains -Wno-error for several of the insn-* files, but
after further investigation I think with this patch we won't need them anymore.
I'm not removing it until I bootstrap gcc on more arches, though. Meanwhile,
this patch at least makes the code more robust.
Richard S., can you look at the genattrtab.c bit? Thanks,
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2016-09-27 Marek Polacek <polacek@redhat.com>
* config/i386/sse.md: Add FALLTHRU markers.
* genattrtab.c (write_attr_case): Also emit FALLTHRU marker.
diff --git gcc/config/i386/sse.md gcc/config/i386/sse.md
index 7e718a0..a503b19 100644
--- gcc/config/i386/sse.md
+++ gcc/config/i386/sse.md
@@ -11268,8 +11268,10 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
@@ -11298,8 +11300,10 @@
case MODE_V16SF:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_V8SF:
gcc_assert (TARGET_AVX);
+ /* FALLTHRU */
case MODE_V4SF:
gcc_assert (TARGET_SSE);
@@ -11393,8 +11397,10 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ /* FALLTHRU */
case MODE_TI:
gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
switch (<MODE>mode)
@@ -11402,10 +11408,11 @@
case V16SImode:
case V8DImode:
if (TARGET_AVX512F)
- {
- tmp = "p<logic><ssemodesuffix>";
- break;
- }
+ {
+ tmp = "p<logic><ssemodesuffix>";
+ break;
+ }
+ /* FALLTHRU */
case V8SImode:
case V4DImode:
case V4SImode:
@@ -11419,6 +11426,7 @@
case MODE_V8SF:
gcc_assert (TARGET_AVX);
+ /* FALLTHRU */
case MODE_V4SF:
gcc_assert (TARGET_SSE);
gcc_assert (!<mask_applied>);
@@ -11489,8 +11497,10 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ /* FALLTHRU */
case MODE_TI:
gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
switch (<MODE>mode)
@@ -11498,21 +11508,23 @@
case V64QImode:
case V32HImode:
if (TARGET_AVX512F)
- {
- tmp = "p<logic>";
- ssesuffix = "q";
- break;
- }
+ {
+ tmp = "p<logic>";
+ ssesuffix = "q";
+ break;
+ }
+ /* FALLTHRU */
case V32QImode:
case V16HImode:
case V16QImode:
case V8HImode:
if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
- {
- tmp = "p<logic>";
- ssesuffix = TARGET_AVX512VL ? "q" : "";
- break;
- }
+ {
+ tmp = "p<logic>";
+ ssesuffix = TARGET_AVX512VL ? "q" : "";
+ break;
+ }
+ /* FALLTHRU */
default:
gcc_unreachable ();
}
@@ -11520,6 +11532,7 @@
case MODE_V8SF:
gcc_assert (TARGET_AVX);
+ /* FALLTHRU */
case MODE_V4SF:
gcc_assert (TARGET_SSE);
tmp = "<logic>ps";
diff --git gcc/genattrtab.c gcc/genattrtab.c
index c8e166e..3b47f34 100644
--- gcc/genattrtab.c
+++ gcc/genattrtab.c
@@ -4283,6 +4283,8 @@ write_attr_case (FILE *outf, struct attr_desc *attr, struct attr_value *av,
fprintf (outf, " && asm_noperands (PATTERN (insn)) < 0)\n");
write_indent (outf, indent + 2);
fprintf (outf, " fatal_insn_not_found (insn);\n");
+ write_indent (outf, indent + 2);
+ fprintf (outf, "/* FALLTHRU */\n");
}
if (write_case_lines)
Marek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 16:55 PATCH to add more FALLTHRU markers Marek Polacek
@ 2016-09-27 19:00 ` Richard Sandiford
2016-09-29 17:22 ` Marek Polacek
2016-09-27 19:08 ` Florian Weimer
1 sibling, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2016-09-27 19:00 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Richard Sandiford
Marek Polacek <polacek@redhat.com> writes:
> Currently Makefile.in contains -Wno-error for several of the insn-* files, but
> after further investigation I think with this patch we won't need them anymore.
> I'm not removing it until I bootstrap gcc on more arches, though. Meanwhile,
> this patch at least makes the code more robust.
>
> Richard S., can you look at the genattrtab.c bit? Thanks,
genattrab.c part is OK, thanks.
Richard
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 16:55 PATCH to add more FALLTHRU markers Marek Polacek
2016-09-27 19:00 ` Richard Sandiford
@ 2016-09-27 19:08 ` Florian Weimer
2016-09-27 19:22 ` Marek Polacek
1 sibling, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2016-09-27 19:08 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Richard Sandiford
* Marek Polacek:
> @@ -11498,21 +11508,23 @@
> case V64QImode:
> case V32HImode:
> if (TARGET_AVX512F)
> - {
> - tmp = "p<logic>";
> - ssesuffix = "q";
> - break;
> - }
> + {
> + tmp = "p<logic>";
> + ssesuffix = "q";
> + break;
> + }
> + /* FALLTHRU */
> case V32QImode:
> case V16HImode:
> case V16QImode:
> case V8HImode:
> if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
> - {
> - tmp = "p<logic>";
> - ssesuffix = TARGET_AVX512VL ? "q" : "";
> - break;
> - }
> + {
> + tmp = "p<logic>";
> + ssesuffix = TARGET_AVX512VL ? "q" : "";
> + break;
> + }
> + /* FALLTHRU */
> default:
> gcc_unreachable ();
> }
Why isn't this a bug? Wouldn't we want to reach gcc_unreachable ()
if, for example !TARGET_AVX512F and TARGET_SSE2?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 19:08 ` Florian Weimer
@ 2016-09-27 19:22 ` Marek Polacek
2016-09-27 19:33 ` Florian Weimer
0 siblings, 1 reply; 12+ messages in thread
From: Marek Polacek @ 2016-09-27 19:22 UTC (permalink / raw)
To: Florian Weimer; +Cc: GCC Patches, Richard Sandiford
On Tue, Sep 27, 2016 at 08:58:59PM +0200, Florian Weimer wrote:
> * Marek Polacek:
>
> > @@ -11498,21 +11508,23 @@
> > case V64QImode:
> > case V32HImode:
> > if (TARGET_AVX512F)
> > - {
> > - tmp = "p<logic>";
> > - ssesuffix = "q";
> > - break;
> > - }
> > + {
> > + tmp = "p<logic>";
> > + ssesuffix = "q";
> > + break;
> > + }
> > + /* FALLTHRU */
> > case V32QImode:
> > case V16HImode:
> > case V16QImode:
> > case V8HImode:
> > if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
> > - {
> > - tmp = "p<logic>";
> > - ssesuffix = TARGET_AVX512VL ? "q" : "";
> > - break;
> > - }
> > + {
> > + tmp = "p<logic>";
> > + ssesuffix = TARGET_AVX512VL ? "q" : "";
> > + break;
> > + }
> > + /* FALLTHRU */
> > default:
> > gcc_unreachable ();
> > }
>
> Why isn't this a bug? Wouldn't we want to reach gcc_unreachable ()
> if, for example !TARGET_AVX512F and TARGET_SSE2?
?? In that case the FALLTHRU should be there if it's intentional that
we may fall through to default. Otherwise not sure what you mean.
Marek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 19:22 ` Marek Polacek
@ 2016-09-27 19:33 ` Florian Weimer
2016-09-27 19:39 ` Marek Polacek
2016-09-27 20:13 ` Jakub Jelinek
0 siblings, 2 replies; 12+ messages in thread
From: Florian Weimer @ 2016-09-27 19:33 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Richard Sandiford
* Marek Polacek:
> On Tue, Sep 27, 2016 at 08:58:59PM +0200, Florian Weimer wrote:
>> * Marek Polacek:
>>
>> > @@ -11498,21 +11508,23 @@
>> > case V64QImode:
>> > case V32HImode:
>> > if (TARGET_AVX512F)
>> > - {
>> > - tmp = "p<logic>";
>> > - ssesuffix = "q";
>> > - break;
>> > - }
>> > + {
>> > + tmp = "p<logic>";
>> > + ssesuffix = "q";
>> > + break;
>> > + }
>> > + /* FALLTHRU */
>> > case V32QImode:
>> > case V16HImode:
>> > case V16QImode:
>> > case V8HImode:
>> > if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
>> > - {
>> > - tmp = "p<logic>";
>> > - ssesuffix = TARGET_AVX512VL ? "q" : "";
>> > - break;
>> > - }
>> > + {
>> > + tmp = "p<logic>";
>> > + ssesuffix = TARGET_AVX512VL ? "q" : "";
>> > + break;
>> > + }
>> > + /* FALLTHRU */
>> > default:
>> > gcc_unreachable ();
>> > }
>>
>> Why isn't this a bug? Wouldn't we want to reach gcc_unreachable ()
>> if, for example !TARGET_AVX512F and TARGET_SSE2?
>
> ?? In that case the FALLTHRU should be there if it's intentional that
> we may fall through to default. Otherwise not sure what you mean.
Not sure if I read this code correctly, but if we fall through from
V32HImode, and we have TARGET_SSE2 set, we execute this code:
tmp = "p<logic>";
ssesuffix = TARGET_AVX512VL ? "q" : "";
And not gcc_unreachable (), as is probably intended.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 19:33 ` Florian Weimer
@ 2016-09-27 19:39 ` Marek Polacek
2016-09-27 20:13 ` Jakub Jelinek
1 sibling, 0 replies; 12+ messages in thread
From: Marek Polacek @ 2016-09-27 19:39 UTC (permalink / raw)
To: Florian Weimer; +Cc: GCC Patches, Richard Sandiford, Kyrylo Tkachov
On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
> * Marek Polacek:
>
> > On Tue, Sep 27, 2016 at 08:58:59PM +0200, Florian Weimer wrote:
> >> * Marek Polacek:
> >>
> >> > @@ -11498,21 +11508,23 @@
> >> > case V64QImode:
> >> > case V32HImode:
> >> > if (TARGET_AVX512F)
> >> > - {
> >> > - tmp = "p<logic>";
> >> > - ssesuffix = "q";
> >> > - break;
> >> > - }
> >> > + {
> >> > + tmp = "p<logic>";
> >> > + ssesuffix = "q";
> >> > + break;
> >> > + }
> >> > + /* FALLTHRU */
> >> > case V32QImode:
> >> > case V16HImode:
> >> > case V16QImode:
> >> > case V8HImode:
> >> > if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
> >> > - {
> >> > - tmp = "p<logic>";
> >> > - ssesuffix = TARGET_AVX512VL ? "q" : "";
> >> > - break;
> >> > - }
> >> > + {
> >> > + tmp = "p<logic>";
> >> > + ssesuffix = TARGET_AVX512VL ? "q" : "";
> >> > + break;
> >> > + }
> >> > + /* FALLTHRU */
> >> > default:
> >> > gcc_unreachable ();
> >> > }
> >>
> >> Why isn't this a bug? Wouldn't we want to reach gcc_unreachable ()
> >> if, for example !TARGET_AVX512F and TARGET_SSE2?
> >
> > ?? In that case the FALLTHRU should be there if it's intentional that
> > we may fall through to default. Otherwise not sure what you mean.
>
> Not sure if I read this code correctly, but if we fall through from
> V32HImode, and we have TARGET_SSE2 set, we execute this code:
>
> tmp = "p<logic>";
> ssesuffix = TARGET_AVX512VL ? "q" : "";
>
> And not gcc_unreachable (), as is probably intended.
Kyrill, can you please decide? Thanks.
Marek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 19:33 ` Florian Weimer
2016-09-27 19:39 ` Marek Polacek
@ 2016-09-27 20:13 ` Jakub Jelinek
2016-09-29 17:17 ` Marek Polacek
1 sibling, 1 reply; 12+ messages in thread
From: Jakub Jelinek @ 2016-09-27 20:13 UTC (permalink / raw)
To: Florian Weimer; +Cc: Marek Polacek, GCC Patches, Richard Sandiford
[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]
On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
> Not sure if I read this code correctly, but if we fall through from
> V32HImode, and we have TARGET_SSE2 set, we execute this code:
>
> tmp = "p<logic>";
> ssesuffix = TARGET_AVX512VL ? "q" : "";
>
> And not gcc_unreachable (), as is probably intended.
It really doesn't matter.
The instruction uses
(define_mode_iterator VI12_AVX_AVX512F
[ (V64QI "TARGET_AVX512F") (V32QI "TARGET_AVX") V16QI
(V32HI "TARGET_AVX512F") (V16HI "TARGET_AVX") V8HI])
iterator (and, after all, ix86_hard_regno_mode_ok should ensure the same),
which means V64QI/V32HI will only show up for TARGET_AVX512F, V32QI/V16HI
only for TARGET_AVX (which implies TARGET_SSE2), and the slightly
nonsensical
gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
before the switch (the || TARGET_AVX512VL is pointless, because
TARGET_AVX512VL implies TARGET_SSE2 as well as TARGET_AVX2).
So, I'd go perhaps for (untested) following patch, first diff -up, followed
by diff -upb:
Jakub
[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 2837 bytes --]
--- gcc/config/i386/sse.md 2016-08-30 08:42:09.169067639 +0200
+++ gcc/config/i386/sse.md 2016-09-27 21:56:29.093582896 +0200
@@ -11393,28 +11393,27 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
- {
- case V16SImode:
- case V8DImode:
- if (TARGET_AVX512F)
- {
- tmp = "p<logic><ssemodesuffix>";
- break;
- }
- case V8SImode:
- case V4DImode:
- case V4SImode:
- case V2DImode:
- tmp = TARGET_AVX512VL ? "p<logic><ssemodesuffix>" : "p<logic>";
- break;
- default:
- gcc_unreachable ();
- }
+ {
+ case V16SImode:
+ case V8DImode:
+ tmp = "p<logic><ssemodesuffix>";
+ break;
+ case V8SImode:
+ case V4DImode:
+ case V4SImode:
+ case V2DImode:
+ tmp = TARGET_AVX512VL ? "p<logic><ssemodesuffix>" : "p<logic>";
+ break;
+ default:
+ gcc_unreachable ();
+ }
break;
case MODE_V8SF:
@@ -11489,45 +11488,41 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
- {
- case V64QImode:
- case V32HImode:
- if (TARGET_AVX512F)
- {
- tmp = "p<logic>";
- ssesuffix = "q";
- break;
- }
- case V32QImode:
- case V16HImode:
- case V16QImode:
- case V8HImode:
- if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
- {
- tmp = "p<logic>";
- ssesuffix = TARGET_AVX512VL ? "q" : "";
- break;
- }
- default:
- gcc_unreachable ();
- }
+ {
+ case V64QImode:
+ case V32HImode:
+ tmp = "p<logic>";
+ ssesuffix = "q";
+ break;
+ case V32QImode:
+ case V16HImode:
+ case V16QImode:
+ case V8HImode:
+ tmp = "p<logic>";
+ ssesuffix = TARGET_AVX512VL ? "q" : "";
+ break;
+ default:
+ gcc_unreachable ();
+ }
break;
case MODE_V8SF:
- gcc_assert (TARGET_AVX);
+ gcc_assert (TARGET_AVX);
case MODE_V4SF:
- gcc_assert (TARGET_SSE);
- tmp = "<logic>ps";
- ssesuffix = "";
- break;
+ gcc_assert (TARGET_SSE);
+ tmp = "<logic>ps";
+ ssesuffix = "";
+ break;
default:
- gcc_unreachable ();
+ gcc_unreachable ();
}
switch (which_alternative)
[-- Attachment #3: 2 --]
[-- Type: text/plain, Size: 1661 bytes --]
--- gcc/config/i386/sse.md 2016-08-30 08:42:09.169067639 +0200
+++ gcc/config/i386/sse.md 2016-09-27 21:56:29.093582896 +0200
@@ -11393,19 +11393,18 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
{
case V16SImode:
case V8DImode:
- if (TARGET_AVX512F)
- {
tmp = "p<logic><ssemodesuffix>";
break;
- }
case V8SImode:
case V4DImode:
case V4SImode:
@@ -11489,30 +11488,26 @@
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
{
case V64QImode:
case V32HImode:
- if (TARGET_AVX512F)
- {
tmp = "p<logic>";
ssesuffix = "q";
break;
- }
case V32QImode:
case V16HImode:
case V16QImode:
case V8HImode:
- if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
- {
tmp = "p<logic>";
ssesuffix = TARGET_AVX512VL ? "q" : "";
break;
- }
default:
gcc_unreachable ();
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 20:13 ` Jakub Jelinek
@ 2016-09-29 17:17 ` Marek Polacek
2016-09-29 20:21 ` Jakub Jelinek
2016-09-30 21:31 ` [PATCH] FALLTHRU markers for sse.md and some cleanups Jakub Jelinek
0 siblings, 2 replies; 12+ messages in thread
From: Marek Polacek @ 2016-09-29 17:17 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Florian Weimer, GCC Patches, Richard Sandiford
On Tue, Sep 27, 2016 at 09:58:20PM +0200, Jakub Jelinek wrote:
> On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
> > Not sure if I read this code correctly, but if we fall through from
> > V32HImode, and we have TARGET_SSE2 set, we execute this code:
> >
> > tmp = "p<logic>";
> > ssesuffix = TARGET_AVX512VL ? "q" : "";
> >
> > And not gcc_unreachable (), as is probably intended.
>
> It really doesn't matter.
> The instruction uses
> (define_mode_iterator VI12_AVX_AVX512F
> [ (V64QI "TARGET_AVX512F") (V32QI "TARGET_AVX") V16QI
> (V32HI "TARGET_AVX512F") (V16HI "TARGET_AVX") V8HI])
> iterator (and, after all, ix86_hard_regno_mode_ok should ensure the same),
> which means V64QI/V32HI will only show up for TARGET_AVX512F, V32QI/V16HI
> only for TARGET_AVX (which implies TARGET_SSE2), and the slightly
> nonsensical
> gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
> before the switch (the || TARGET_AVX512VL is pointless, because
> TARGET_AVX512VL implies TARGET_SSE2 as well as TARGET_AVX2).
> So, I'd go perhaps for (untested) following patch, first diff -up, followed
> by diff -upb:
Looks good, are you going to test/commit it? Or should I?
Marek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-27 19:00 ` Richard Sandiford
@ 2016-09-29 17:22 ` Marek Polacek
0 siblings, 0 replies; 12+ messages in thread
From: Marek Polacek @ 2016-09-29 17:22 UTC (permalink / raw)
To: GCC Patches, Richard Sandiford, rdsandiford
On Tue, Sep 27, 2016 at 07:57:17PM +0100, Richard Sandiford wrote:
> Marek Polacek <polacek@redhat.com> writes:
> > Currently Makefile.in contains -Wno-error for several of the insn-* files, but
> > after further investigation I think with this patch we won't need them anymore.
> > I'm not removing it until I bootstrap gcc on more arches, though. Meanwhile,
> > this patch at least makes the code more robust.
> >
> > Richard S., can you look at the genattrtab.c bit? Thanks,
>
> genattrab.c part is OK, thanks.
Thanks. I'm going to commit this part separately.
Marek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PATCH to add more FALLTHRU markers
2016-09-29 17:17 ` Marek Polacek
@ 2016-09-29 20:21 ` Jakub Jelinek
2016-09-30 21:31 ` [PATCH] FALLTHRU markers for sse.md and some cleanups Jakub Jelinek
1 sibling, 0 replies; 12+ messages in thread
From: Jakub Jelinek @ 2016-09-29 20:21 UTC (permalink / raw)
To: Marek Polacek; +Cc: Florian Weimer, GCC Patches, Richard Sandiford
On Thu, Sep 29, 2016 at 06:21:13PM +0200, Marek Polacek wrote:
> On Tue, Sep 27, 2016 at 09:58:20PM +0200, Jakub Jelinek wrote:
> > On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
> > > Not sure if I read this code correctly, but if we fall through from
> > > V32HImode, and we have TARGET_SSE2 set, we execute this code:
> > >
> > > tmp = "p<logic>";
> > > ssesuffix = TARGET_AVX512VL ? "q" : "";
> > >
> > > And not gcc_unreachable (), as is probably intended.
> >
> > It really doesn't matter.
> > The instruction uses
> > (define_mode_iterator VI12_AVX_AVX512F
> > [ (V64QI "TARGET_AVX512F") (V32QI "TARGET_AVX") V16QI
> > (V32HI "TARGET_AVX512F") (V16HI "TARGET_AVX") V8HI])
> > iterator (and, after all, ix86_hard_regno_mode_ok should ensure the same),
> > which means V64QI/V32HI will only show up for TARGET_AVX512F, V32QI/V16HI
> > only for TARGET_AVX (which implies TARGET_SSE2), and the slightly
> > nonsensical
> > gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
> > before the switch (the || TARGET_AVX512VL is pointless, because
> > TARGET_AVX512VL implies TARGET_SSE2 as well as TARGET_AVX2).
> > So, I'd go perhaps for (untested) following patch, first diff -up, followed
> > by diff -upb:
>
> Looks good, are you going to test/commit it? Or should I?
Forgot to test it, will do tomorrow.
Jakub
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] FALLTHRU markers for sse.md and some cleanups
2016-09-29 17:17 ` Marek Polacek
2016-09-29 20:21 ` Jakub Jelinek
@ 2016-09-30 21:31 ` Jakub Jelinek
2016-10-01 8:31 ` Uros Bizjak
1 sibling, 1 reply; 12+ messages in thread
From: Jakub Jelinek @ 2016-09-30 21:31 UTC (permalink / raw)
To: Uros Bizjak, Kirill Yukhin, Marek Polacek; +Cc: gcc-patches
On Thu, Sep 29, 2016 at 06:21:13PM +0200, Marek Polacek wrote:
> On Tue, Sep 27, 2016 at 09:58:20PM +0200, Jakub Jelinek wrote:
> > On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
> > > Not sure if I read this code correctly, but if we fall through from
> > > V32HImode, and we have TARGET_SSE2 set, we execute this code:
> > >
> > > tmp = "p<logic>";
> > > ssesuffix = TARGET_AVX512VL ? "q" : "";
> > >
> > > And not gcc_unreachable (), as is probably intended.
> >
> > It really doesn't matter.
> > The instruction uses
> > (define_mode_iterator VI12_AVX_AVX512F
> > [ (V64QI "TARGET_AVX512F") (V32QI "TARGET_AVX") V16QI
> > (V32HI "TARGET_AVX512F") (V16HI "TARGET_AVX") V8HI])
> > iterator (and, after all, ix86_hard_regno_mode_ok should ensure the same),
> > which means V64QI/V32HI will only show up for TARGET_AVX512F, V32QI/V16HI
> > only for TARGET_AVX (which implies TARGET_SSE2), and the slightly
> > nonsensical
> > gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
> > before the switch (the || TARGET_AVX512VL is pointless, because
> > TARGET_AVX512VL implies TARGET_SSE2 as well as TARGET_AVX2).
> > So, I'd go perhaps for (untested) following patch, first diff -up, followed
> > by diff -upb:
>
> Looks good, are you going to test/commit it? Or should I?
Here it is, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2016-09-30 Jakub Jelinek <jakub@redhat.com>
* config/i386/sse.md (<mask_codefor><code><mode>): Add FALLTHRU
comments. Simplify asserts, remove unnecessary conditions.
Formatting fixes.
(*<code><mode>3): Likewise.
--- gcc/config/i386/sse.md.jj 2016-08-30 08:42:09.169067639 +0200
+++ gcc/config/i386/sse.md 2016-09-30 14:40:45.382959729 +0200
@@ -11393,41 +11393,40 @@ (define_insn "<mask_codefor><code><mode>
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
- {
- case V16SImode:
- case V8DImode:
- if (TARGET_AVX512F)
- {
- tmp = "p<logic><ssemodesuffix>";
- break;
- }
- case V8SImode:
- case V4DImode:
- case V4SImode:
- case V2DImode:
- tmp = TARGET_AVX512VL ? "p<logic><ssemodesuffix>" : "p<logic>";
- break;
- default:
- gcc_unreachable ();
- }
+ {
+ case V16SImode:
+ case V8DImode:
+ tmp = "p<logic><ssemodesuffix>";
+ break;
+ case V8SImode:
+ case V4DImode:
+ case V4SImode:
+ case V2DImode:
+ tmp = TARGET_AVX512VL ? "p<logic><ssemodesuffix>" : "p<logic>";
+ break;
+ default:
+ gcc_unreachable ();
+ }
break;
- case MODE_V8SF:
+ case MODE_V8SF:
gcc_assert (TARGET_AVX);
- case MODE_V4SF:
+ case MODE_V4SF:
gcc_assert (TARGET_SSE);
gcc_assert (!<mask_applied>);
tmp = "<logic>ps";
break;
- default:
+ default:
gcc_unreachable ();
- }
+ }
switch (which_alternative)
{
@@ -11489,46 +11488,42 @@ (define_insn "*<code><mode>3"
{
case MODE_XI:
gcc_assert (TARGET_AVX512F);
+ /* FALLTHRU */
case MODE_OI:
- gcc_assert (TARGET_AVX2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_AVX2);
+ /* FALLTHRU */
case MODE_TI:
- gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
+ gcc_assert (TARGET_SSE2);
switch (<MODE>mode)
- {
- case V64QImode:
- case V32HImode:
- if (TARGET_AVX512F)
- {
- tmp = "p<logic>";
- ssesuffix = "q";
- break;
- }
- case V32QImode:
- case V16HImode:
- case V16QImode:
- case V8HImode:
- if (TARGET_AVX512VL || TARGET_AVX2 || TARGET_SSE2)
- {
- tmp = "p<logic>";
- ssesuffix = TARGET_AVX512VL ? "q" : "";
- break;
- }
- default:
- gcc_unreachable ();
- }
+ {
+ case V64QImode:
+ case V32HImode:
+ tmp = "p<logic>";
+ ssesuffix = "q";
+ break;
+ case V32QImode:
+ case V16HImode:
+ case V16QImode:
+ case V8HImode:
+ tmp = "p<logic>";
+ ssesuffix = TARGET_AVX512VL ? "q" : "";
+ break;
+ default:
+ gcc_unreachable ();
+ }
break;
- case MODE_V8SF:
+ case MODE_V8SF:
gcc_assert (TARGET_AVX);
- case MODE_V4SF:
+ case MODE_V4SF:
gcc_assert (TARGET_SSE);
tmp = "<logic>ps";
ssesuffix = "";
break;
- default:
+ default:
gcc_unreachable ();
- }
+ }
switch (which_alternative)
{
Jakub
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] FALLTHRU markers for sse.md and some cleanups
2016-09-30 21:31 ` [PATCH] FALLTHRU markers for sse.md and some cleanups Jakub Jelinek
@ 2016-10-01 8:31 ` Uros Bizjak
0 siblings, 0 replies; 12+ messages in thread
From: Uros Bizjak @ 2016-10-01 8:31 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Kirill Yukhin, Marek Polacek, gcc-patches
On Fri, Sep 30, 2016 at 10:17 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Sep 29, 2016 at 06:21:13PM +0200, Marek Polacek wrote:
>> On Tue, Sep 27, 2016 at 09:58:20PM +0200, Jakub Jelinek wrote:
>> > On Tue, Sep 27, 2016 at 09:29:10PM +0200, Florian Weimer wrote:
>> > > Not sure if I read this code correctly, but if we fall through from
>> > > V32HImode, and we have TARGET_SSE2 set, we execute this code:
>> > >
>> > > tmp = "p<logic>";
>> > > ssesuffix = TARGET_AVX512VL ? "q" : "";
>> > >
>> > > And not gcc_unreachable (), as is probably intended.
>> >
>> > It really doesn't matter.
>> > The instruction uses
>> > (define_mode_iterator VI12_AVX_AVX512F
>> > [ (V64QI "TARGET_AVX512F") (V32QI "TARGET_AVX") V16QI
>> > (V32HI "TARGET_AVX512F") (V16HI "TARGET_AVX") V8HI])
>> > iterator (and, after all, ix86_hard_regno_mode_ok should ensure the same),
>> > which means V64QI/V32HI will only show up for TARGET_AVX512F, V32QI/V16HI
>> > only for TARGET_AVX (which implies TARGET_SSE2), and the slightly
>> > nonsensical
>> > gcc_assert (TARGET_SSE2 || TARGET_AVX512VL);
>> > before the switch (the || TARGET_AVX512VL is pointless, because
>> > TARGET_AVX512VL implies TARGET_SSE2 as well as TARGET_AVX2).
>> > So, I'd go perhaps for (untested) following patch, first diff -up, followed
>> > by diff -upb:
>>
>> Looks good, are you going to test/commit it? Or should I?
>
> Here it is, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
>
> 2016-09-30 Jakub Jelinek <jakub@redhat.com>
>
> * config/i386/sse.md (<mask_codefor><code><mode>): Add FALLTHRU
> comments. Simplify asserts, remove unnecessary conditions.
> Formatting fixes.
> (*<code><mode>3): Likewise.
OK.
Thanks,
Uros.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-01 8:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-27 16:55 PATCH to add more FALLTHRU markers Marek Polacek
2016-09-27 19:00 ` Richard Sandiford
2016-09-29 17:22 ` Marek Polacek
2016-09-27 19:08 ` Florian Weimer
2016-09-27 19:22 ` Marek Polacek
2016-09-27 19:33 ` Florian Weimer
2016-09-27 19:39 ` Marek Polacek
2016-09-27 20:13 ` Jakub Jelinek
2016-09-29 17:17 ` Marek Polacek
2016-09-29 20:21 ` Jakub Jelinek
2016-09-30 21:31 ` [PATCH] FALLTHRU markers for sse.md and some cleanups Jakub Jelinek
2016-10-01 8:31 ` Uros Bizjak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).