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

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.

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

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

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

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