| Message ID | CAKdteOY2Y-HNAMabBkKDo_t8gSyATTyyhvYDv8o3KScnJ5iZEQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [armeb] PR 91060 gcc.c-torture/execute/scal-to-vec1.c fails since r272843 | expand |
Christophe Lyon <christophe.lyon@linaro.org> writes: > Hi, > > This patch fixes PR 91060 where the lane ordering was no longer the > right one (GCC's vs architecture's). Sorry, we clashed :-) I'd prefer to go with the version I attached to bugzilla just now. Thanks, Richard
Hi Christophe On 7/8/19 10:01 AM, Christophe Lyon wrote: > Hi, > > This patch fixes PR 91060 where the lane ordering was no longer the > right one (GCC's vs architecture's). > > OK? > > Thanks to both Richards for most of the debugging! Thank you to all for tracking this down. > > Christophe pr91060.patch.txt diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 820502a..4c7b5a8 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -12471,7 +12471,7 @@ neon_expand_vector_init (rtx target, rtx vals) if (n_var == 1) { rtx copy = copy_rtx (vals); - rtx index = GEN_INT (one_var); + rtx index = GEN_INT (1 << one_var); /* Load constant part of vector, substitute neighboring value for varying element. */ @@ -12483,31 +12483,31 @@ neon_expand_vector_init (rtx target, rtx vals) switch (mode) { case E_V8QImode: - emit_insn (gen_neon_vset_lanev8qi (target, x, target, index)); + emit_insn (gen_vec_setv8qi_internal (target, x, index, target)); break; case E_V16QImode: - emit_insn (gen_neon_vset_lanev16qi (target, x, target, index)); + emit_insn (gen_vec_setv16qi_internal (target, x, index, target)); break; case E_V4HImode: - emit_insn (gen_neon_vset_lanev4hi (target, x, target, index)); + emit_insn (gen_vec_setv4hi_internal (target, x, index, target)); break; case E_V8HImode: - emit_insn (gen_neon_vset_lanev8hi (target, x, target, index)); + emit_insn (gen_vec_setv8hi_internal (target, x, index, target)); break; case E_V2SImode: - emit_insn (gen_neon_vset_lanev2si (target, x, target, index)); + emit_insn (gen_vec_setv2si_internal (target, x, index, target)); break; case E_V4SImode: - emit_insn (gen_neon_vset_lanev4si (target, x, target, index)); + emit_insn (gen_vec_setv4si_internal (target, x, index, target)); break; case E_V2SFmode: - emit_insn (gen_neon_vset_lanev2sf (target, x, target, index)); + emit_insn (gen_vec_setv2sf_internal (target, x, index, target)); break; case E_V4SFmode: - emit_insn (gen_neon_vset_lanev4sf (target, x, target, index)); + emit_insn (gen_vec_setv4sf_internal (target, x, index, target)); break; case E_V2DImode: - emit_insn (gen_neon_vset_lanev2di (target, x, target, index)); + emit_insn (gen_vec_setv2di_internal (target, x, index, target)); break; default: gcc_unreachable (); Can we take the opportunity here to remove that switch statement and use the parametrised names machinery: https://gcc.gnu.org/onlinedocs/gccint/Parameterized-Names.html#Parameterized-Names so that we can instead have one call to gen_vec_setv8hi_internal (mode, target, x, merge_mask, target) or something. Thanks, Kyrill
On Mon, 8 Jul 2019 at 11:04, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > Hi, > > > > This patch fixes PR 91060 where the lane ordering was no longer the > > right one (GCC's vs architecture's). > > Sorry, we clashed :-) > > I'd prefer to go with the version I attached to bugzilla just now. Yes just saw that, thanks! > > Thanks, > Richard
On Mon, 8 Jul 2019 at 11:06, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > > Hi Christophe > > On 7/8/19 10:01 AM, Christophe Lyon wrote: > > Hi, > > > > This patch fixes PR 91060 where the lane ordering was no longer the > > right one (GCC's vs architecture's). > > > > OK? > > > > Thanks to both Richards for most of the debugging! > > Thank you to all for tracking this down. > > > > > Christophe > > > pr91060.patch.txt > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index 820502a..4c7b5a8 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -12471,7 +12471,7 @@ neon_expand_vector_init (rtx target, rtx vals) > if (n_var == 1) > { > rtx copy = copy_rtx (vals); > - rtx index = GEN_INT (one_var); > + rtx index = GEN_INT (1 << one_var); > > /* Load constant part of vector, substitute neighboring value for > varying element. */ > @@ -12483,31 +12483,31 @@ neon_expand_vector_init (rtx target, rtx vals) > switch (mode) > { > case E_V8QImode: > - emit_insn (gen_neon_vset_lanev8qi (target, x, target, index)); > + emit_insn (gen_vec_setv8qi_internal (target, x, index, target)); > break; > case E_V16QImode: > - emit_insn (gen_neon_vset_lanev16qi (target, x, target, index)); > + emit_insn (gen_vec_setv16qi_internal (target, x, index, target)); > break; > case E_V4HImode: > - emit_insn (gen_neon_vset_lanev4hi (target, x, target, index)); > + emit_insn (gen_vec_setv4hi_internal (target, x, index, target)); > break; > case E_V8HImode: > - emit_insn (gen_neon_vset_lanev8hi (target, x, target, index)); > + emit_insn (gen_vec_setv8hi_internal (target, x, index, target)); > break; > case E_V2SImode: > - emit_insn (gen_neon_vset_lanev2si (target, x, target, index)); > + emit_insn (gen_vec_setv2si_internal (target, x, index, target)); > break; > case E_V4SImode: > - emit_insn (gen_neon_vset_lanev4si (target, x, target, index)); > + emit_insn (gen_vec_setv4si_internal (target, x, index, target)); > break; > case E_V2SFmode: > - emit_insn (gen_neon_vset_lanev2sf (target, x, target, index)); > + emit_insn (gen_vec_setv2sf_internal (target, x, index, target)); > break; > case E_V4SFmode: > - emit_insn (gen_neon_vset_lanev4sf (target, x, target, index)); > + emit_insn (gen_vec_setv4sf_internal (target, x, index, target)); > break; > case E_V2DImode: > - emit_insn (gen_neon_vset_lanev2di (target, x, target, index)); > + emit_insn (gen_vec_setv2di_internal (target, x, index, target)); > break; > default: > gcc_unreachable (); > > > Can we take the opportunity here to remove that switch statement and use the parametrised names machinery: > https://gcc.gnu.org/onlinedocs/gccint/Parameterized-Names.html#Parameterized-Names > > so that we can instead have one call to gen_vec_setv8hi_internal (mode, target, x, merge_mask, target) or something. Yes, that's what Richard posted in bugzilla, it's much nicer indeed. > Thanks, > Kyrill >
Christophe Lyon <christophe.lyon@linaro.org> writes: > On Mon, 8 Jul 2019 at 11:04, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Christophe Lyon <christophe.lyon@linaro.org> writes: >> > Hi, >> > >> > This patch fixes PR 91060 where the lane ordering was no longer the >> > right one (GCC's vs architecture's). >> >> Sorry, we clashed :-) >> >> I'd prefer to go with the version I attached to bugzilla just now. > > Yes just saw that, thanks! The bugzilla version didn't properly adjust vec_setv2di_internal. Fixed with the version below, tested on armeb-eabi. Besides gcc.c-torture/execute/scal-to-vec*.c, the patch also fixes: c-c++-common/torture/vector-compare-1.c gcc.target/arm/pr69614.c g++.dg/ext/vector37.C OK for trunk? Richard 2019-07-10 Richard Sandiford <richard.sandiford@arm.com> gcc/ PR target/91060 * config/arm/iterators.md (V2DI_ONLY): New mode iterator. * config/arm/neon.md (vec_set<mode>_internal): Add a '@' prefix. (vec_setv2di_internal): Reexpress as... (@vec_set<V2DI_ONLY:mode>_internal): ...this. * config/arm/arm.c (neon_expand_vector_init): Use gen_vec_set_internal rather than gen_neon_vset_lane<mode>. Index: gcc/config/arm/iterators.md =================================================================== --- gcc/config/arm/iterators.md 2019-06-18 09:35:55.377865698 +0100 +++ gcc/config/arm/iterators.md 2019-07-10 11:01:57.990749932 +0100 @@ -186,6 +186,9 @@ (define_mode_iterator VX [V8QI V4HI V16Q ;; Modes with 8-bit elements. (define_mode_iterator VE [V8QI V16QI]) +;; V2DI only (for use with @ patterns). +(define_mode_iterator V2DI_ONLY [V2DI]) + ;; Modes with 64-bit elements only. (define_mode_iterator V64 [DI V2DI]) Index: gcc/config/arm/neon.md =================================================================== --- gcc/config/arm/neon.md 2019-07-01 09:37:07.220524486 +0100 +++ gcc/config/arm/neon.md 2019-07-10 11:01:57.990749932 +0100 @@ -319,7 +319,7 @@ (define_insn "*movmisalign<mode>_neon_lo "vld1.<V_sz_elem>\t{%q0}, %A1" [(set_attr "type" "neon_load1_1reg<q>")]) -(define_insn "vec_set<mode>_internal" +(define_insn "@vec_set<mode>_internal" [(set (match_operand:VD_LANE 0 "s_register_operand" "=w,w") (vec_merge:VD_LANE (vec_duplicate:VD_LANE @@ -340,7 +340,7 @@ (define_insn "vec_set<mode>_internal" } [(set_attr "type" "neon_load1_all_lanes<q>,neon_from_gp<q>")]) -(define_insn "vec_set<mode>_internal" +(define_insn "@vec_set<mode>_internal" [(set (match_operand:VQ2 0 "s_register_operand" "=w,w") (vec_merge:VQ2 (vec_duplicate:VQ2 @@ -369,12 +369,12 @@ (define_insn "vec_set<mode>_internal" [(set_attr "type" "neon_load1_all_lanes<q>,neon_from_gp<q>")] ) -(define_insn "vec_setv2di_internal" - [(set (match_operand:V2DI 0 "s_register_operand" "=w,w") - (vec_merge:V2DI - (vec_duplicate:V2DI +(define_insn "@vec_set<mode>_internal" + [(set (match_operand:V2DI_ONLY 0 "s_register_operand" "=w,w") + (vec_merge:V2DI_ONLY + (vec_duplicate:V2DI_ONLY (match_operand:DI 1 "nonimmediate_operand" "Um,r")) - (match_operand:V2DI 3 "s_register_operand" "0,0") + (match_operand:V2DI_ONLY 3 "s_register_operand" "0,0") (match_operand:SI 2 "immediate_operand" "i,i")))] "TARGET_NEON" { Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c 2019-07-01 09:37:07.220524486 +0100 +++ gcc/config/arm/arm.c 2019-07-10 11:01:57.990749932 +0100 @@ -12471,7 +12471,7 @@ neon_expand_vector_init (rtx target, rtx if (n_var == 1) { rtx copy = copy_rtx (vals); - rtx index = GEN_INT (one_var); + rtx merge_mask = GEN_INT (1 << one_var); /* Load constant part of vector, substitute neighboring value for varying element. */ @@ -12480,38 +12480,7 @@ neon_expand_vector_init (rtx target, rtx /* Insert variable. */ x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, one_var)); - switch (mode) - { - case E_V8QImode: - emit_insn (gen_neon_vset_lanev8qi (target, x, target, index)); - break; - case E_V16QImode: - emit_insn (gen_neon_vset_lanev16qi (target, x, target, index)); - break; - case E_V4HImode: - emit_insn (gen_neon_vset_lanev4hi (target, x, target, index)); - break; - case E_V8HImode: - emit_insn (gen_neon_vset_lanev8hi (target, x, target, index)); - break; - case E_V2SImode: - emit_insn (gen_neon_vset_lanev2si (target, x, target, index)); - break; - case E_V4SImode: - emit_insn (gen_neon_vset_lanev4si (target, x, target, index)); - break; - case E_V2SFmode: - emit_insn (gen_neon_vset_lanev2sf (target, x, target, index)); - break; - case E_V4SFmode: - emit_insn (gen_neon_vset_lanev4sf (target, x, target, index)); - break; - case E_V2DImode: - emit_insn (gen_neon_vset_lanev2di (target, x, target, index)); - break; - default: - gcc_unreachable (); - } + emit_insn (gen_vec_set_internal (mode, target, x, merge_mask, target)); return; }
On Wed, Jul 10, 2019 at 11:07 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > On Mon, 8 Jul 2019 at 11:04, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Christophe Lyon <christophe.lyon@linaro.org> writes: > >> > Hi, > >> > > >> > This patch fixes PR 91060 where the lane ordering was no longer the > >> > right one (GCC's vs architecture's). > >> > >> Sorry, we clashed :-) > >> > >> I'd prefer to go with the version I attached to bugzilla just now. > > > > Yes just saw that, thanks! > > The bugzilla version didn't properly adjust vec_setv2di_internal. > Fixed with the version below, tested on armeb-eabi. > > Besides gcc.c-torture/execute/scal-to-vec*.c, the patch also fixes: > > c-c++-common/torture/vector-compare-1.c > gcc.target/arm/pr69614.c > g++.dg/ext/vector37.C > > OK for trunk? OK. Ramana > > Richard > > > 2019-07-10 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > PR target/91060 > * config/arm/iterators.md (V2DI_ONLY): New mode iterator. > * config/arm/neon.md (vec_set<mode>_internal): Add a '@' prefix. > (vec_setv2di_internal): Reexpress as... > (@vec_set<V2DI_ONLY:mode>_internal): ...this. > * config/arm/arm.c (neon_expand_vector_init): Use gen_vec_set_internal > rather than gen_neon_vset_lane<mode>. > > Index: gcc/config/arm/iterators.md > =================================================================== > --- gcc/config/arm/iterators.md 2019-06-18 09:35:55.377865698 +0100 > +++ gcc/config/arm/iterators.md 2019-07-10 11:01:57.990749932 +0100 > @@ -186,6 +186,9 @@ (define_mode_iterator VX [V8QI V4HI V16Q > ;; Modes with 8-bit elements. > (define_mode_iterator VE [V8QI V16QI]) > > +;; V2DI only (for use with @ patterns). > +(define_mode_iterator V2DI_ONLY [V2DI]) > + > ;; Modes with 64-bit elements only. > (define_mode_iterator V64 [DI V2DI]) > > Index: gcc/config/arm/neon.md > =================================================================== > --- gcc/config/arm/neon.md 2019-07-01 09:37:07.220524486 +0100 > +++ gcc/config/arm/neon.md 2019-07-10 11:01:57.990749932 +0100 > @@ -319,7 +319,7 @@ (define_insn "*movmisalign<mode>_neon_lo > "vld1.<V_sz_elem>\t{%q0}, %A1" > [(set_attr "type" "neon_load1_1reg<q>")]) > > -(define_insn "vec_set<mode>_internal" > +(define_insn "@vec_set<mode>_internal" > [(set (match_operand:VD_LANE 0 "s_register_operand" "=w,w") > (vec_merge:VD_LANE > (vec_duplicate:VD_LANE > @@ -340,7 +340,7 @@ (define_insn "vec_set<mode>_internal" > } > [(set_attr "type" "neon_load1_all_lanes<q>,neon_from_gp<q>")]) > > -(define_insn "vec_set<mode>_internal" > +(define_insn "@vec_set<mode>_internal" > [(set (match_operand:VQ2 0 "s_register_operand" "=w,w") > (vec_merge:VQ2 > (vec_duplicate:VQ2 > @@ -369,12 +369,12 @@ (define_insn "vec_set<mode>_internal" > [(set_attr "type" "neon_load1_all_lanes<q>,neon_from_gp<q>")] > ) > > -(define_insn "vec_setv2di_internal" > - [(set (match_operand:V2DI 0 "s_register_operand" "=w,w") > - (vec_merge:V2DI > - (vec_duplicate:V2DI > +(define_insn "@vec_set<mode>_internal" > + [(set (match_operand:V2DI_ONLY 0 "s_register_operand" "=w,w") > + (vec_merge:V2DI_ONLY > + (vec_duplicate:V2DI_ONLY > (match_operand:DI 1 "nonimmediate_operand" "Um,r")) > - (match_operand:V2DI 3 "s_register_operand" "0,0") > + (match_operand:V2DI_ONLY 3 "s_register_operand" "0,0") > (match_operand:SI 2 "immediate_operand" "i,i")))] > "TARGET_NEON" > { > Index: gcc/config/arm/arm.c > =================================================================== > --- gcc/config/arm/arm.c 2019-07-01 09:37:07.220524486 +0100 > +++ gcc/config/arm/arm.c 2019-07-10 11:01:57.990749932 +0100 > @@ -12471,7 +12471,7 @@ neon_expand_vector_init (rtx target, rtx > if (n_var == 1) > { > rtx copy = copy_rtx (vals); > - rtx index = GEN_INT (one_var); > + rtx merge_mask = GEN_INT (1 << one_var); > > /* Load constant part of vector, substitute neighboring value for > varying element. */ > @@ -12480,38 +12480,7 @@ neon_expand_vector_init (rtx target, rtx > > /* Insert variable. */ > x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, one_var)); > - switch (mode) > - { > - case E_V8QImode: > - emit_insn (gen_neon_vset_lanev8qi (target, x, target, index)); > - break; > - case E_V16QImode: > - emit_insn (gen_neon_vset_lanev16qi (target, x, target, index)); > - break; > - case E_V4HImode: > - emit_insn (gen_neon_vset_lanev4hi (target, x, target, index)); > - break; > - case E_V8HImode: > - emit_insn (gen_neon_vset_lanev8hi (target, x, target, index)); > - break; > - case E_V2SImode: > - emit_insn (gen_neon_vset_lanev2si (target, x, target, index)); > - break; > - case E_V4SImode: > - emit_insn (gen_neon_vset_lanev4si (target, x, target, index)); > - break; > - case E_V2SFmode: > - emit_insn (gen_neon_vset_lanev2sf (target, x, target, index)); > - break; > - case E_V4SFmode: > - emit_insn (gen_neon_vset_lanev4sf (target, x, target, index)); > - break; > - case E_V2DImode: > - emit_insn (gen_neon_vset_lanev2di (target, x, target, index)); > - break; > - default: > - gcc_unreachable (); > - } > + emit_insn (gen_vec_set_internal (mode, target, x, merge_mask, target)); > return; > } >
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 820502a..4c7b5a8 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -12471,7 +12471,7 @@ neon_expand_vector_init (rtx target, rtx vals) if (n_var == 1) { rtx copy = copy_rtx (vals); - rtx index = GEN_INT (one_var); + rtx index = GEN_INT (1 << one_var); /* Load constant part of vector, substitute neighboring value for varying element. */ @@ -12483,31 +12483,31 @@ neon_expand_vector_init (rtx target, rtx vals) switch (mode) { case E_V8QImode: - emit_insn (gen_neon_vset_lanev8qi (target, x, target, index)); + emit_insn (gen_vec_setv8qi_internal (target, x, index, target)); break; case E_V16QImode: - emit_insn (gen_neon_vset_lanev16qi (target, x, target, index)); + emit_insn (gen_vec_setv16qi_internal (target, x, index, target)); break; case E_V4HImode: - emit_insn (gen_neon_vset_lanev4hi (target, x, target, index)); + emit_insn (gen_vec_setv4hi_internal (target, x, index, target)); break; case E_V8HImode: - emit_insn (gen_neon_vset_lanev8hi (target, x, target, index)); + emit_insn (gen_vec_setv8hi_internal (target, x, index, target)); break; case E_V2SImode: - emit_insn (gen_neon_vset_lanev2si (target, x, target, index)); + emit_insn (gen_vec_setv2si_internal (target, x, index, target)); break; case E_V4SImode: - emit_insn (gen_neon_vset_lanev4si (target, x, target, index)); + emit_insn (gen_vec_setv4si_internal (target, x, index, target)); break; case E_V2SFmode: - emit_insn (gen_neon_vset_lanev2sf (target, x, target, index)); + emit_insn (gen_vec_setv2sf_internal (target, x, index, target)); break; case E_V4SFmode: - emit_insn (gen_neon_vset_lanev4sf (target, x, target, index)); + emit_insn (gen_vec_setv4sf_internal (target, x, index, target)); break; case E_V2DImode: - emit_insn (gen_neon_vset_lanev2di (target, x, target, index)); + emit_insn (gen_vec_setv2di_internal (target, x, index, target)); break; default: gcc_unreachable ();