[08/11,rs6000] Fix ambiguous .md attribute uses

Message ID mpto928wlru.fsf@arm.com
State New
Headers show
Series
  • Diagnose ambiguous .md attribute uses
Related show

Commit Message

Richard Sandiford July 5, 2019, 3:20 p.m.
This patch is part of a series that fixes ambiguous attribute
uses in .md files, i.e. cases in which attributes didn't use
<ITER:ATTR> to specify an iterator, and in which <ATTR> could
have different values depending on the iterator chosen.

No behavioural change -- produces the same code as before except
for formatting and line numbers.


2019-07-05  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* config/rs6000/rs6000.md (*mov<mode>_update1): Explicitly
	use <SFDF:mode>, <SFDF:MODE>, <SFDF:Ff> and <SFDF:bits> rather than
	leaving the choice between SFDF and P implicit.
	(*mov<mode>_update2): Likewise.
	(*cmp<IBM128:mode>_internal2): Explicitly use <IBM128:MODE>
	rather than leaving the choice betweem IBM128 and GPR implicit.
	(*fix<uns>_trunc<IEEE128:mode><QHSI:mode>2_mem): Explicitly use
	<IEEE128:MODE> rather than leaving the choice between IEEE128 and
	QHSI implicit.
	(AltiVec define_peephole2s): Explicitly use <ALTIVEC_DFORM:MODE>
	rather than leaving the choice between ALTIVEC_DFORM and P implicit.
	* config/rs6000/vsx.md
	(*vsx_ext_<VSX_EXTRACT_I:VS_scalar>_fl_<FL_CONV:mode>)
	(*vsx_ext_<VSX_EXTRACT_I:VS_scalar>_ufl_<FL_CONV:mode>): Explicitly
	use <FL_CONV:VSisa> rather than leaving the choice between FL_CONV
	and VSX_EXTRACT_I implicit.

Comments

Segher Boessenkool July 5, 2019, 3:59 p.m. | #1
Hi Richard,

On Fri, Jul 05, 2019 at 04:20:37PM +0100, Richard Sandiford wrote:
> This patch is part of a series that fixes ambiguous attribute

> uses in .md files, i.e. cases in which attributes didn't use

> <ITER:ATTR> to specify an iterator, and in which <ATTR> could

> have different values depending on the iterator chosen.


I have sometimes wondered which iterator is chosen in ambiguous cases.
So I finally looked it up, and the doc says
  The @code{@var{iterator}:} prefix may be omitted, in which case the
  substitution will be attempted for every iterator expansion.

Well ugh :-)  I always thought there was some rule, but nope.

Maybe there should be some way of indicating what iterator you want if
none is mentioned?  For the whole pattern, or some global priority scheme
even?  Changes like (random example)

> -(define_insn "*mov<mode>_update2"

> +(define_insn "*mov<SFDF:mode>_update2"

>    [(set (mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")

>  			  (match_operand:P 2 "reg_or_short_operand" "r,I")))

> -	(match_operand:SFDF 3 "gpc_reg_operand" "<Ff>,<Ff>"))

> +	(match_operand:SFDF 3 "gpc_reg_operand" "<SFDF:Ff>,<SFDF:Ff>"))

>     (set (match_operand:P 0 "gpc_reg_operand" "=b,b")

>  	(plus:P (match_dup 1) (match_dup 2)))]

>    "TARGET_HARD_FLOAT && TARGET_UPDATE

> -   && (!avoiding_indexed_address_p (<MODE>mode)

> +   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)


do not make the code more readable.  A rule like "Do not use P if any
other mode iterator is available" would work for us.


(The patch is fine if the whole series is approved, of course).

Thanks,


Segher
Richard Sandiford July 5, 2019, 4:26 p.m. | #2
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi Richard,

>

> On Fri, Jul 05, 2019 at 04:20:37PM +0100, Richard Sandiford wrote:

>> This patch is part of a series that fixes ambiguous attribute

>> uses in .md files, i.e. cases in which attributes didn't use

>> <ITER:ATTR> to specify an iterator, and in which <ATTR> could

>> have different values depending on the iterator chosen.

>

> I have sometimes wondered which iterator is chosen in ambiguous cases.

> So I finally looked it up, and the doc says

>   The @code{@var{iterator}:} prefix may be omitted, in which case the

>   substitution will be attempted for every iterator expansion.

>

> Well ugh :-)  I always thought there was some rule, but nope.

>

> Maybe there should be some way of indicating what iterator you want if

> none is mentioned?  For the whole pattern, or some global priority scheme

> even?  Changes like (random example)

>

>> -(define_insn "*mov<mode>_update2"

>> +(define_insn "*mov<SFDF:mode>_update2"

>>    [(set (mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")

>>  			  (match_operand:P 2 "reg_or_short_operand" "r,I")))

>> -	(match_operand:SFDF 3 "gpc_reg_operand" "<Ff>,<Ff>"))

>> +	(match_operand:SFDF 3 "gpc_reg_operand" "<SFDF:Ff>,<SFDF:Ff>"))

>>     (set (match_operand:P 0 "gpc_reg_operand" "=b,b")

>>  	(plus:P (match_dup 1) (match_dup 2)))]

>>    "TARGET_HARD_FLOAT && TARGET_UPDATE

>> -   && (!avoiding_indexed_address_p (<MODE>mode)

>> +   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)

>

> do not make the code more readable.  A rule like "Do not use P if any

> other mode iterator is available" would work for us.


Yeah, it's a bit ugly, but I think it's better to be explicit even
for P.  E.g. there are pattern names like:

    vsx_extract_<P:mode>_<VSX_D:mode>_load

in which using that rule and dropping the iterator names would silently
do something unexpected.  (Not a big deal for "*" patterns of course.)

But I think the bigger problem is that attributes tend to grow over
time, and that something that used to be unambiguous can suddenly
become ambiguous without anyone noticing.  E.g. an attribute A might
start with just SI and DI, and so unambiguously refer to P in a PxVECTOR
pattern.  But then it might be useful to add vector modes to A for some
unrelated pattern.

So even if it the order was selectable, it would still be easy to get
things wrong when you're not thinking about it.  And the series is only
forcing an iterator to be specified if there's genuine ambiguity.
E.g. if Ff was specific to floating-point modes, using <Ff> would
still be OK.

> (The patch is fine if the whole series is approved, of course).


Thanks!

Richard

> Thanks,

>

>

> Segher
Segher Boessenkool July 5, 2019, 5:23 p.m. | #3
On Fri, Jul 05, 2019 at 05:26:24PM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:

> > Maybe there should be some way of indicating what iterator you want if

> > none is mentioned?  For the whole pattern, or some global priority scheme

> > even?  Changes like (random example)

> >

> >> -(define_insn "*mov<mode>_update2"

> >> +(define_insn "*mov<SFDF:mode>_update2"

> >>    [(set (mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")

> >>  			  (match_operand:P 2 "reg_or_short_operand" "r,I")))

> >> -	(match_operand:SFDF 3 "gpc_reg_operand" "<Ff>,<Ff>"))

> >> +	(match_operand:SFDF 3 "gpc_reg_operand" "<SFDF:Ff>,<SFDF:Ff>"))

> >>     (set (match_operand:P 0 "gpc_reg_operand" "=b,b")

> >>  	(plus:P (match_dup 1) (match_dup 2)))]

> >>    "TARGET_HARD_FLOAT && TARGET_UPDATE

> >> -   && (!avoiding_indexed_address_p (<MODE>mode)

> >> +   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)

> >

> > do not make the code more readable.  A rule like "Do not use P if any

> > other mode iterator is available" would work for us.

> 

> Yeah, it's a bit ugly, but I think it's better to be explicit even

> for P.  E.g. there are pattern names like:

> 

>     vsx_extract_<P:mode>_<VSX_D:mode>_load

> 

> in which using that rule and dropping the iterator names would silently

> do something unexpected.  (Not a big deal for "*" patterns of course.)


This would be

     vsx_extract_<P:mode>_<mode>_load

in that case.  It *can* still be confused, sure.

> But I think the bigger problem is that attributes tend to grow over

> time, and that something that used to be unambiguous can suddenly

> become ambiguous without anyone noticing.  E.g. an attribute A might

> start with just SI and DI, and so unambiguously refer to P in a PxVECTOR

> pattern.  But then it might be useful to add vector modes to A for some

> unrelated pattern.

> 

> So even if it the order was selectable, it would still be easy to get

> things wrong when you're not thinking about it.  And the series is only

> forcing an iterator to be specified if there's genuine ambiguity.


Yeah.  And there aren't very many places being explicit is needed.  Do
you have some estimate how much that "only disallow if actually ambiguous"
helped?  I expected more changes needed :-)

> E.g. if Ff was specific to floating-point modes, using <Ff> would

> still be OK.


Our Ff unfortunately is not for FP modes only, but for modes that can go
in FP registers :-/


Segher
Richard Sandiford July 5, 2019, 5:33 p.m. | #4
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Fri, Jul 05, 2019 at 05:26:24PM +0100, Richard Sandiford wrote:

>> Segher Boessenkool <segher@kernel.crashing.org> writes:

>> > Maybe there should be some way of indicating what iterator you want if

>> > none is mentioned?  For the whole pattern, or some global priority scheme

>> > even?  Changes like (random example)

>> >

>> >> -(define_insn "*mov<mode>_update2"

>> >> +(define_insn "*mov<SFDF:mode>_update2"

>> >>    [(set (mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")

>> >>  			  (match_operand:P 2 "reg_or_short_operand" "r,I")))

>> >> -	(match_operand:SFDF 3 "gpc_reg_operand" "<Ff>,<Ff>"))

>> >> +	(match_operand:SFDF 3 "gpc_reg_operand" "<SFDF:Ff>,<SFDF:Ff>"))

>> >>     (set (match_operand:P 0 "gpc_reg_operand" "=b,b")

>> >>  	(plus:P (match_dup 1) (match_dup 2)))]

>> >>    "TARGET_HARD_FLOAT && TARGET_UPDATE

>> >> -   && (!avoiding_indexed_address_p (<MODE>mode)

>> >> +   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)

>> >

>> > do not make the code more readable.  A rule like "Do not use P if any

>> > other mode iterator is available" would work for us.

>> 

>> Yeah, it's a bit ugly, but I think it's better to be explicit even

>> for P.  E.g. there are pattern names like:

>> 

>>     vsx_extract_<P:mode>_<VSX_D:mode>_load

>> 

>> in which using that rule and dropping the iterator names would silently

>> do something unexpected.  (Not a big deal for "*" patterns of course.)

>

> This would be

>

>      vsx_extract_<P:mode>_<mode>_load

>

> in that case.  It *can* still be confused, sure.

>

>> But I think the bigger problem is that attributes tend to grow over

>> time, and that something that used to be unambiguous can suddenly

>> become ambiguous without anyone noticing.  E.g. an attribute A might

>> start with just SI and DI, and so unambiguously refer to P in a PxVECTOR

>> pattern.  But then it might be useful to add vector modes to A for some

>> unrelated pattern.

>> 

>> So even if it the order was selectable, it would still be easy to get

>> things wrong when you're not thinking about it.  And the series is only

>> forcing an iterator to be specified if there's genuine ambiguity.

>

> Yeah.  And there aren't very many places being explicit is needed.  Do

> you have some estimate how much that "only disallow if actually ambiguous"

> helped?  I expected more changes needed :-)


No real estimate, but I imagine loads :-)  Having two mode iterators
is pretty common, and forcing a qualifier even when there's no
ambiguity (e.g. using vector queries in a vector x P pattern)
would probably be over the top.

And yeah, I was pleasantly surprised how few places needed changing.
The bug-fix to make-work ratio seemed pretty high in the end (but not
for rs6000).

Richard

>

>> E.g. if Ff was specific to floating-point modes, using <Ff> would

>> still be OK.

>

> Our Ff unfortunately is not for FP modes only, but for modes that can go

> in FP registers :-/

>

>

> Segher

Patch

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	2019-07-03 20:50:46.390316903 +0100
+++ gcc/config/rs6000/rs6000.md	2019-07-05 15:08:06.639145562 +0100
@@ -9312,14 +9312,14 @@  (define_insn "*movqi_update3"
    (set_attr "update" "yes")
    (set_attr "indexed" "yes,no")])
 
-(define_insn "*mov<mode>_update1"
-  [(set (match_operand:SFDF 3 "gpc_reg_operand" "=<Ff>,<Ff>")
+(define_insn "*mov<SFDF:mode>_update1"
+  [(set (match_operand:SFDF 3 "gpc_reg_operand" "=<SFDF:Ff>,<SFDF:Ff>")
 	(mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")
 			  (match_operand:P 2 "reg_or_short_operand" "r,I"))))
    (set (match_operand:P 0 "gpc_reg_operand" "=b,b")
 	(plus:P (match_dup 1) (match_dup 2)))]
   "TARGET_HARD_FLOAT && TARGET_UPDATE
-   && (!avoiding_indexed_address_p (<MODE>mode)
+   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)
        || !gpc_reg_operand (operands[2], Pmode))"
   "@
    lf<sd>ux %3,%0,%2
@@ -9327,16 +9327,16 @@  (define_insn "*mov<mode>_update1"
   [(set_attr "type" "fpload")
    (set_attr "update" "yes")
    (set_attr "indexed" "yes,no")
-   (set_attr "size" "<bits>")])
+   (set_attr "size" "<SFDF:bits>")])
 
-(define_insn "*mov<mode>_update2"
+(define_insn "*mov<SFDF:mode>_update2"
   [(set (mem:SFDF (plus:P (match_operand:P 1 "gpc_reg_operand" "0,0")
 			  (match_operand:P 2 "reg_or_short_operand" "r,I")))
-	(match_operand:SFDF 3 "gpc_reg_operand" "<Ff>,<Ff>"))
+	(match_operand:SFDF 3 "gpc_reg_operand" "<SFDF:Ff>,<SFDF:Ff>"))
    (set (match_operand:P 0 "gpc_reg_operand" "=b,b")
 	(plus:P (match_dup 1) (match_dup 2)))]
   "TARGET_HARD_FLOAT && TARGET_UPDATE
-   && (!avoiding_indexed_address_p (<MODE>mode)
+   && (!avoiding_indexed_address_p (<SFDF:MODE>mode)
        || !gpc_reg_operand (operands[2], Pmode))"
   "@
    stf<sd>ux %3,%0,%2
@@ -9344,7 +9344,7 @@  (define_insn "*mov<mode>_update2"
   [(set_attr "type" "fpstore")
    (set_attr "update" "yes")
    (set_attr "indexed" "yes,no")
-   (set_attr "size" "<bits>")])
+   (set_attr "size" "<SFDF:bits>")])
 
 (define_insn "*movsf_update3"
   [(set (match_operand:SF 3 "gpc_reg_operand" "=r,r")
@@ -11558,7 +11558,7 @@  (define_insn "*cmp<mode>_internal1"
   [(set_attr "type" "fpcompare")
    (set_attr "length" "12")])
 
-(define_insn_and_split "*cmp<mode>_internal2"
+(define_insn_and_split "*cmp<IBM128:mode>_internal2"
   [(set (match_operand:CCFP 0 "cc_reg_operand" "=y")
 	(compare:CCFP (match_operand:IBM128 1 "gpc_reg_operand" "d")
 		      (match_operand:IBM128 2 "gpc_reg_operand" "d")))
@@ -11571,7 +11571,7 @@  (define_insn_and_split "*cmp<mode>_inter
     (clobber (match_scratch:DF 9 "=d"))
     (clobber (match_scratch:DF 10 "=d"))
     (clobber (match_scratch:GPR 11 "=b"))]
-  "TARGET_XL_COMPAT && FLOAT128_IBM_P (<MODE>mode)
+  "TARGET_XL_COMPAT && FLOAT128_IBM_P (<IBM128:MODE>mode)
    && TARGET_HARD_FLOAT && TARGET_LONG_DOUBLE_128"
   "#"
   "&& reload_completed"
@@ -11595,10 +11595,14 @@  (define_insn_and_split "*cmp<mode>_inter
   const int lo_word = LONG_DOUBLE_LARGE_FIRST ? GET_MODE_SIZE (DFmode) : 0;
   const int hi_word = LONG_DOUBLE_LARGE_FIRST ? 0 : GET_MODE_SIZE (DFmode);
 
-  operands[5] = simplify_gen_subreg (DFmode, operands[1], <MODE>mode, hi_word);
-  operands[6] = simplify_gen_subreg (DFmode, operands[1], <MODE>mode, lo_word);
-  operands[7] = simplify_gen_subreg (DFmode, operands[2], <MODE>mode, hi_word);
-  operands[8] = simplify_gen_subreg (DFmode, operands[2], <MODE>mode, lo_word);
+  operands[5] = simplify_gen_subreg (DFmode, operands[1],
+				     <IBM128:MODE>mode, hi_word);
+  operands[6] = simplify_gen_subreg (DFmode, operands[1],
+				     <IBM128:MODE>mode, lo_word);
+  operands[7] = simplify_gen_subreg (DFmode, operands[2],
+				     <IBM128:MODE>mode, hi_word);
+  operands[8] = simplify_gen_subreg (DFmode, operands[2],
+				     <IBM128:MODE>mode, lo_word);
   operands[12] = gen_label_rtx ();
   operands[13] = gen_label_rtx ();
   real_inf (&rv);
@@ -13597,7 +13601,7 @@  (define_peephole2
   new_addr = gen_rtx_PLUS (Pmode, add_op0, tmp_reg);
 
   operands[4] = add_op1;
-  operands[5] = change_address (mem, <MODE>mode, new_addr);
+  operands[5] = change_address (mem, <ALTIVEC_DFORM:MODE>mode, new_addr);
 })
 
 ;; Optimize cases were want to do a D-form store on ISA 2.06/2.07 from an
@@ -13633,7 +13637,7 @@  (define_peephole2
   new_addr = gen_rtx_PLUS (Pmode, add_op0, tmp_reg);
 
   operands[4] = add_op1;
-  operands[5] = change_address (mem, <MODE>mode, new_addr);
+  operands[5] = change_address (mem, <ALTIVEC_DFORM:MODE>mode, new_addr);
 })
    
 
@@ -14073,7 +14077,7 @@  (define_insn_and_split "*fix<uns>_trunc<
 	(any_fix:QHSI
 	 (match_operand:IEEE128 1 "altivec_register_operand" "v")))
    (clobber (match_scratch:QHSI 2 "=v"))]
-  "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)"
+  "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<IEEE128:MODE>mode)"
   "#"
   "&& reload_completed"
   [(set (match_dup 2)
Index: gcc/config/rs6000/vsx.md
===================================================================
--- gcc/config/rs6000/vsx.md	2019-07-03 20:50:46.390316903 +0100
+++ gcc/config/rs6000/vsx.md	2019-07-05 15:08:06.639145562 +0100
@@ -3828,7 +3828,7 @@  (define_insn_and_split "*vsx_ext_<VSX_EX
 {
   operands[4] = gen_rtx_REG (DImode, REGNO (operands[3]));
 }
-  [(set_attr "isa" "<VSisa>")])
+  [(set_attr "isa" "<FL_CONV:VSisa>")])
 
 (define_insn_and_split "*vsx_ext_<VSX_EXTRACT_I:VS_scalar>_ufl_<FL_CONV:mode>"
   [(set (match_operand:FL_CONV 0 "gpc_reg_operand" "=wa")
@@ -3851,7 +3851,7 @@  (define_insn_and_split "*vsx_ext_<VSX_EX
 {
   operands[4] = gen_rtx_REG (DImode, REGNO (operands[3]));
 }
-  [(set_attr "isa" "<VSisa>")])
+  [(set_attr "isa" "<FL_CONV:VSisa>")])
 
 ;; V4SI/V8HI/V16QI set operation on ISA 3.0
 (define_insn "vsx_set_<mode>_p9"