Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/22/18 6:03 PM, Segher Boessenkool wrote: > On Wed, Mar 21, 2018 at 09:10:38PM -0500, Peter Bergner wrote: >> If you're asking about whether we also need to backport to GCC 6, I >> believe Kaushik said in the bugzilla that he only encountered the >> ICE on GCC 7 and trunk, so I don't think we need a GCC 6 backport. > > Uh yes, that is what I meant. Okay, since we have no way to check if > it fixes anything, let's not backport it to 6. Ok, backport committed to the GCC 7 release branch. Thanks. Peter
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On Wed, Mar 21, 2018 at 09:10:38PM -0500, Peter Bergner wrote: > On 3/21/18 7:37 PM, Segher Boessenkool wrote: > > On Wed, Mar 21, 2018 at 12:47:41PM -0500, Peter Bergner wrote: > >> I'll note that GCC 7 does not need any of the changes to rs6000-p8swap.c, > >> since that file doesn't exist and doesn't need to exist in GCC 7, so I've > >> left those changes out. > > > > Did that same code live in rs6000.c in GCC 7, or is it new since > > rs6000-p8swap.c was split off from there? And what about GCC 6? > > The trunk patch's changes to rs6000-p8swap.c were to rs6000_gen_lvx and > rs6000_gen_stvx and those are new to GCC 8. They are used as part of > Kelvin's optimization that changes the little endian unfriendly vsx > loads and stores that require swaps, with the altivec lvx/stvx which > do not need swaps. That optimization was new to GCC 8, so there was > no need for the rs6000-p8swap.c changes. Thanks for looking. > If you're asking about whether we also need to backport to GCC 6, I > believe Kaushik said in the bugzilla that he only encountered the > ICE on GCC 7 and trunk, so I don't think we need a GCC 6 backport. Uh yes, that is what I meant. Okay, since we have no way to check if it fixes anything, let's not backport it to 6. Segher
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/21/18 12:47 PM, Peter Bergner wrote: > Kaushik confirmed this is broken on GCC 7 as well. Ok to backport > this patch to GCC 7, assuming regtesting is clean? FYI, bootstrap and regtesting on both powerpc64le-linux and powerpc64-linux (testsuite run in both 32-bit and 64-bit modes) were clean. Peter
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/21/18 7:37 PM, Segher Boessenkool wrote: > On Wed, Mar 21, 2018 at 12:47:41PM -0500, Peter Bergner wrote: >> I'll note that GCC 7 does not need any of the changes to rs6000-p8swap.c, >> since that file doesn't exist and doesn't need to exist in GCC 7, so I've >> left those changes out. > > Did that same code live in rs6000.c in GCC 7, or is it new since > rs6000-p8swap.c was split off from there? And what about GCC 6? The trunk patch's changes to rs6000-p8swap.c were to rs6000_gen_lvx and rs6000_gen_stvx and those are new to GCC 8. They are used as part of Kelvin's optimization that changes the little endian unfriendly vsx loads and stores that require swaps, with the altivec lvx/stvx which do not need swaps. That optimization was new to GCC 8, so there was no need for the rs6000-p8swap.c changes. If you're asking about whether we also need to backport to GCC 6, I believe Kaushik said in the bugzilla that he only encountered the ICE on GCC 7 and trunk, so I don't think we need a GCC 6 backport. Peter
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On Wed, Mar 21, 2018 at 12:47:41PM -0500, Peter Bergner wrote: > On 3/20/18 12:27 PM, Peter Bergner wrote: > > On 3/20/18 11:42 AM, Segher Boessenkool wrote: > >> On Mon, Mar 19, 2018 at 09:12:08PM -0500, Peter Bergner wrote: > >>> Looking at mu build dirs insn-modes.h, I don't see HAVE_V8HFmode being > >>> defined on either my LE or BE builds. What am I missing? > >> > >> Nothing, I just was confused (we always define the mode in > >> rs6000-modes.def, > >> but that is not the same thing). Sorry. > > > > Ok then, patch committed with the expander change you requested and > > leaving the HAVE_V8HFmode code as is in the patch. Thanks! > > Kaushik confirmed this is broken on GCC 7 as well. Ok to backport > this patch to GCC 7, assuming regtesting is clean? > > I'll note that GCC 7 does not need any of the changes to rs6000-p8swap.c, > since that file doesn't exist and doesn't need to exist in GCC 7, so I've > left those changes out. Did that same code live in rs6000.c in GCC 7, or is it new since rs6000-p8swap.c was split off from there? And what about GCC 6? Segher
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/20/18 12:27 PM, Peter Bergner wrote: > On 3/20/18 11:42 AM, Segher Boessenkool wrote: >> On Mon, Mar 19, 2018 at 09:12:08PM -0500, Peter Bergner wrote: >>> Looking at mu build dirs insn-modes.h, I don't see HAVE_V8HFmode being >>> defined on either my LE or BE builds. What am I missing? >> >> Nothing, I just was confused (we always define the mode in rs6000-modes.def, >> but that is not the same thing). Sorry. > > Ok then, patch committed with the expander change you requested and > leaving the HAVE_V8HFmode code as is in the patch. Thanks! Kaushik confirmed this is broken on GCC 7 as well. Ok to backport this patch to GCC 7, assuming regtesting is clean? I'll note that GCC 7 does not need any of the changes to rs6000-p8swap.c, since that file doesn't exist and doesn't need to exist in GCC 7, so I've left those changes out. Peter
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/20/18 11:42 AM, Segher Boessenkool wrote: > On Mon, Mar 19, 2018 at 09:12:08PM -0500, Peter Bergner wrote: >> Looking at mu build dirs insn-modes.h, I don't see HAVE_V8HFmode being >> defined on either my LE or BE builds. What am I missing? > > Nothing, I just was confused (we always define the mode in rs6000-modes.def, > but that is not the same thing). Sorry. Ok then, patch committed with the expander change you requested and leaving the HAVE_V8HFmode code as is in the patch. Thanks! Peter
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On Mon, Mar 19, 2018 at 09:12:08PM -0500, Peter Bergner wrote: > On 3/12/18 1:55 PM, Segher Boessenkool wrote: > >> #ifdef HAVE_V8HFmode > >> - else if (mode == V8HFmode) > >> - stvx = TARGET_64BIT > >> -? gen_altivec_stvx_v8hf_1op (src_exp, memory_address) > >> -: gen_altivec_stvx_v8hf_1op_si (src_exp, memory_address); > >> + else if (mode == V8HFmode) > >> +stvx = gen_altivec_stvx_v8hf (src_exp, dest_exp); > >> #endif > > > > Btw, don't we always have V8HFmode these days? > > I have no idea, I was just working around code that was already there. > Looking at the history, Kelvin seems to have added the tests. > Kelvin, what is the above trying to protect? > > Looking at mu build dirs insn-modes.h, I don't see HAVE_V8HFmode being > defined on either my LE or BE builds. What am I missing? Nothing, I just was confused (we always define the mode in rs6000-modes.def, but that is not the same thing). Sorry. Segher
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
On 3/12/18 1:55 PM, Segher Boessenkool wrote: > On Sun, Mar 11, 2018 at 10:23:02AM -0500, Peter Bergner wrote: >> +; The following patterns embody what lvx should usually look like. >> +(define_expand "altivec_lvx_" >> + [(set (match_operand:VM2 0 "register_operand" "") >> +(match_operand:VM2 1 "altivec_indexed_or_indirect_operand" ""))] > > No "" please. Expanders do not have constraints. Cut & paste error on my part I believe. Will fix. >> #ifdef HAVE_V8HFmode >> - else if (mode == V8HFmode) >> -stvx = TARGET_64BIT >> - ? gen_altivec_stvx_v8hf_1op (src_exp, memory_address) >> - : gen_altivec_stvx_v8hf_1op_si (src_exp, memory_address); >> + else if (mode == V8HFmode) >> +stvx = gen_altivec_stvx_v8hf (src_exp, dest_exp); >> #endif > > Btw, don't we always have V8HFmode these days? I have no idea, I was just working around code that was already there. Looking at the history, Kelvin seems to have added the tests. Kelvin, what is the above trying to protect? Looking at mu build dirs insn-modes.h, I don't see HAVE_V8HFmode being defined on either my LE or BE builds. What am I missing? Peter
RE: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
Hi, Just to update from my side, this patch fixes the issues I had reported in PR83789 and there are no new regression in my testing. Best Regards, Kaushik M. Phatak -Original Message- From: Segher Boessenkool [mailto:seg...@kernel.crashing.org] Sent: Tuesday, March 13, 2018 12:25 AM To: Peter Bergner <berg...@vnet.ibm.com> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; Kaushik Phatak <kaushik.pha...@kpit.com>; Bill Schmidt <wschm...@linux.vnet.ibm.com> Subject: Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c Hi! On Sun, Mar 11, 2018 at 10:23:02AM -0500, Peter Bergner wrote: > PR83789 shows a problem in the builtin expansion code not calling the There is no hurry I think? And some changes are needed, so I'll leave it to you. The patch is fine with those trivialities fixed. Okay for trunk. Thanks! Enjoy your vacation! Segher
Re: [PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
Hi! On Sun, Mar 11, 2018 at 10:23:02AM -0500, Peter Bergner wrote: > PR83789 shows a problem in the builtin expansion code not calling the correct > define_insn, given the correct mode (32-bit versus 64-bit). One could add > tests in this code to call the correct pattern, but it's easier to create > a common define_expand which everyone can call that does the right thing. > This allows us to clean up all the callers, making for much simpler code. > This also fixes the issue that Segher mentioned that this needs fixing for > multiple other vector modes and not just the one mentioned in the bugzilla. > P.S. I will be away on vacation for the neek week, so if this is ok, I won't > be able to commit the patch until I return. Unless you want to commit > it Segher and watch for fallout. It's up to you. There is no hurry I think? And some changes are needed, so I'll leave it to you. > +; The following patterns embody what lvx should usually look like. > +(define_expand "altivec_lvx_" > + [(set (match_operand:VM2 0 "register_operand" "") > + (match_operand:VM2 1 "altivec_indexed_or_indirect_operand" ""))] No "" please. Expanders do not have constraints. > #ifdef HAVE_V8HFmode > - else if (mode == V8HFmode) > - stvx = TARGET_64BIT > - ? gen_altivec_stvx_v8hf_1op (src_exp, memory_address) > - : gen_altivec_stvx_v8hf_1op_si (src_exp, memory_address); > + else if (mode == V8HFmode) > +stvx = gen_altivec_stvx_v8hf (src_exp, dest_exp); > #endif Btw, don't we always have V8HFmode these days? The patch is fine with those trivialities fixed. Okay for trunk. Thanks! Enjoy your vacation! Segher
[PATCH, rs6000] Fix PR83789: __builtin_altivec_lvx fails for powerpc for altivec-4.c
PR83789 shows a problem in the builtin expansion code not calling the correct
define_insn, given the correct mode (32-bit versus 64-bit). One could add
tests in this code to call the correct pattern, but it's easier to create
a common define_expand which everyone can call that does the right thing.
This allows us to clean up all the callers, making for much simpler code.
This also fixes the issue that Segher mentioned that this needs fixing for
multiple other vector modes and not just the one mentioned in the bugzilla.
This passed bootstrap and regtesting on powerpc64le-linux and powerpc64-linux
(running testsuite in both 32-bit and 64-bit modes) with no regressions.
Ok for trunk?
I was not able to reproduce the failure reported in the bugzilla, but Kaushik
confirmed that this patch fixes the ICE.
P.S. I will be away on vacation for the neek week, so if this is ok, I won't
be able to commit the patch until I return. Unless you want to commit
it Segher and watch for fallout. It's up to you.
Peter
PR target/83789
* config/rs6000/altivec.md (altivec_lvx__2op): Delete define_insn.
(altivec_lvx__1op): Likewise.
(altivec_stvx__2op): Likewise.
(altivec_stvx__1op): Likewise.
(altivec_lvx_): New define_expand.
(altivec_stvx_): Likewise.
(altivec_lvx__2op_): New define_insn.
(altivec_lvx__1op_): Likewise.
(altivec_stvx__2op_): Likewise.
(altivec_stvx__1op_): Likewise.
* config/rs6000/rs6000-p8swap.c (rs6000_gen_stvx): Use new expanders.
(rs6000_gen_lvx): Likewise.
* config/rs6000/rs6000.c (altivec_expand_lv_builtin): Likewise.
(altivec_expand_stv_builtin): Likewise.
(altivec_expand_builtin): Likewise.
* config/rs6000/vector.md: Likewise.
Index: gcc/config/rs6000/altivec.md
===
--- gcc/config/rs6000/altivec.md(revision 258348)
+++ gcc/config/rs6000/altivec.md(working copy)
@@ -2747,39 +2747,47 @@ (define_insn "altivec_lvx__interna
"lvx %0,%y1"
[(set_attr "type" "vecload")])
-; The next two patterns embody what lvx should usually look like.
-(define_insn "altivec_lvx__2op"
- [(set (match_operand:VM2 0 "register_operand" "=v")
-(mem:VM2 (and:DI (plus:DI (match_operand:DI 1 "register_operand" "b")
- (match_operand:DI 2 "register_operand" "r"))
-(const_int -16]
- "TARGET_ALTIVEC && TARGET_64BIT"
- "lvx %0,%1,%2"
- [(set_attr "type" "vecload")])
-
-(define_insn "altivec_lvx__1op"
- [(set (match_operand:VM2 0 "register_operand" "=v")
-(mem:VM2 (and:DI (match_operand:DI 1 "register_operand" "r")
-(const_int -16]
- "TARGET_ALTIVEC && TARGET_64BIT"
- "lvx %0,0,%1"
- [(set_attr "type" "vecload")])
+; The following patterns embody what lvx should usually look like.
+(define_expand "altivec_lvx_"
+ [(set (match_operand:VM2 0 "register_operand" "")
+ (match_operand:VM2 1 "altivec_indexed_or_indirect_operand" ""))]
+ "TARGET_ALTIVEC"
+{
+ rtx addr = XEXP (operand1, 0);
+ if (rs6000_sum_of_two_registers_p (addr))
+{
+ rtx op1 = XEXP (addr, 0);
+ rtx op2 = XEXP (addr, 1);
+ if (TARGET_64BIT)
+ emit_insn (gen_altivec_lvx__2op_di (operand0, op1, op2));
+ else
+ emit_insn (gen_altivec_lvx__2op_si (operand0, op1, op2));
+}
+ else
+{
+ if (TARGET_64BIT)
+ emit_insn (gen_altivec_lvx__1op_di (operand0, addr));
+ else
+ emit_insn (gen_altivec_lvx__1op_si (operand0, addr));
+}
+ DONE;
+})
-; 32-bit versions of the above.
-(define_insn "altivec_lvx__2op_si"
+; The next two patterns embody what lvx should usually look like.
+(define_insn "altivec_lvx__2op_"
[(set (match_operand:VM2 0 "register_operand" "=v")
-(mem:VM2 (and:SI (plus:SI (match_operand:SI 1 "register_operand" "b")
- (match_operand:SI 2 "register_operand" "r"))
-(const_int -16]
- "TARGET_ALTIVEC && TARGET_32BIT"
+ (mem:VM2 (and:P (plus:P (match_operand:P 1 "register_operand" "b")
+ (match_operand:P 2 "register_operand" "r"))
+ (const_int -16]
+ "TARGET_ALTIVEC"
"lvx %0,%1,%2"
[(set_attr "type" "vecload")])
-(define_insn "altivec_lvx__1op_si"
+(define_insn "altivec_lvx__1op_"
[(set (match_operand:VM2 0 "register_operand" "=v")
-(mem:VM2 (and:SI (match_operand:SI 1 "register_operand" "r")
-(const_int -16]
- "TARGET_ALTIVEC && TARGET_32BIT"
+ (mem:VM2 (and:P (match_operand:P 1 "register_operand" "r")
+ (const_int -16]
+ "TARGET_ALTIVEC"
"lvx %0,0,%1"
[(set_attr "type" "vecload")])
@@ -2795,39 +2803,47 @@ (define_insn "altivec_stvx__intern
"stvx %1,%y0"
[(set_attr "type" "vecstore")])
-; The next two patterns embody what
