| Message ID | 20160927195820.GE7282@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
--- 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)