[PATCH] rs6000: MMA test case ICEs using -O3 [PR99842]

[Segher Boessenkool]

Hi!

On Fri, May 21, 2021 at 02:45:18PM -0500, Peter Bergner wrote:
> Getting back to this now that trunk is open again...
> 
> On 3/31/21 2:17 PM, Segher Boessenkool wrote:
> > On Tue, Mar 30, 2021 at 06:49:29PM -0500, Peter Bergner wrote:
> >> The mma_assemble_input_operand predicate does not accept reg+reg indexed
> >> addresses which can lead to ICEs.  The problem is that the quad_address_p
> >> function only accepts reg+offset addresses that are valid for quad word
> >> accesses, but not reg+reg addresses which are also valid for quad word
> >> accesses when dealing with vector types.  The solution used here is to
> >> call memory_operand, which uses rs6000_legitimate_address_p to ensure
> >> the address is valid.  For reg+offset addresses, it uses quad_address_p like
> >> before, but for reg+reg addresses, it calls legitimate_indexed_address_p
> >> addresses which fixes this specific ICE.
> > 
> > quad_address_p should really only be used for lq/stq (and their prefixed
> > forms).  Those insns have very different semantics: they are atomic,
> > while vector loads and stores are not; and the prefixed form has some
> > special semantics (it swaps halves on LE).
> 
> quad_address_p has since day one been used for both lq/stq as well as
> for vector loads/stores.

The was "quad_memory_operand" in 2013 already, three years earlier, and
it was exclusively for load/store quad insns -- which, as I said, have
very different semantics (they are atomic, they have different alignment
requirements, they have different addressing modes, they have different
semantics in LE -- is there anything the *same* as vector memory
operands even?)

So it would be much clearer and less error-prone if these different
concepts were not artificially put together.  And the name already
suggests it is only for load/store quad insns, not for vectors!

> I think the "quad" here is meant to describe
> that the address will be used for a dform quad word memory access
> (eg, lq/stq, lxv/stxv and lxvp/stxvp) which use DQ offsets.

No.  If you look at history, quad_memory_operand is for lq/stq only.

> I don't
> think quad_address_p cares whether the address is being used for an
> atomic access or not.  That restriction is done elsewhere it seems.

And that is a big part of the problem.

Please don't use "quad" things for vectors at all.  The code will become
simpler, and we might even have a chance of getting it correct!

> rs6000: MMA test case ICEs using -O3 [PR99842]
> 
> The mma_assemble_input_operand predicate does not accept reg+reg indexed
> addresses which can lead to ICEs.  The lxv and lxvp instructions have
> indexed forms (lxvx and lxvpx), so the simple solution is to just allow
> indexed addresses in the predicate.

> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index e21bc745f72..121cbf14810 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1172,7 +1172,8 @@ (define_special_predicate "mma_assemble_input_operand"
>    (match_test "(mode == V16QImode
>  		&& (vsx_register_operand (op, mode)
>  		    || (MEM_P (op)
> -			&& quad_address_p (XEXP (op, 0), mode, false))))"))
> +			&& (indexed_or_indirect_address (XEXP (op, 0), mode)
> +			    || quad_address_p (XEXP (op, 0), mode, false)))))"))

This is okay for trunk and for backports.  But please fix it properly on
trunk after the backport: don't abuse quad* for vectors anymore!

Thanks,


Segher