Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Segher Boessenkool <segher@kernel.crashing.org>]

On Mon, Sep 19, 2022 at 11:13:20AM -0500, will schmidt wrote:
>   The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
> and can be disabled by dependent options when it should not be.
> This manifests in the issue seen in PR101865 where -mno-vsx
> mistakenly disables _ARCH_PWR8.

> This change replaces the relevant TARGET_DIRECT_MOVE references
> with a TARGET_POWER8 entry so that the direct_move and power8
> features can be enabled or disabled independently.

We should get rid of TARGET_DIRECT_MOVE altogether.  Please see
57f108f5a1e1:
    rs6000: Disable -m[no-]direct-move (PR85293)
    
    The -mno-direct-move option causes a lot of problems, since it forces
    us to be able to generate code for p8 and up with some crucial
    instructions missing.  This patch removes the -m[no-]direct-move
    options so that the user cannot put us into this unexpected situation
    anymore.  Internally we still have all the same flags, and they are
    automatically set based on -mcpu; getting rid of that is a lot more
    work and will have to wait for GCC 9 (in some places the flag is used
    to see if we are compiling for a p8 _at all_).

It did not happen in GCC 9 obviously.  Do you want to take a shot?  It
doesn't have to be all at once, it's probably best if not even -- as I
wrote in the commit message, the flag always was used to mean different
things.

> The existing (and rather lengthy) commentary for DIRECT_MOVE remains
> in place in rs6000-c.cc:rs6000_target_modify_macros().  The
> if-defined logic there will now set a __DIRECT_MOVE__ define when
> TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
> purposes, but is otherwise unused.  This can be removed in a
> subsequent patch, or in an update of this patch, depending on feedback.

There should be no such macro, for the same reason there should be no
-mdirect-move option: it is so very essential to all code we generate,
it *always* is enabled if we have P8 or later.

> gcc/
> 	PR Target/101865
> 	* config/rs6000/rs6000-builtin.cc
> 	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
> 	usage with TARGET_POWER8.

Please don't arbitrarily wrap lines.  It is harder to read, and it looks
like something is missing.

> 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
> 	Add OPTION_MASK_POWER8 entry.

Especially in cases like this, where it looks like you forgot to write
something after the colon.

> @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
>  								false, true  },
>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
>    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
> +  { "power8",			OPTION_MASK_POWER8,		false, true  },

Why would we want a #pragma power8 ?

> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -490,10 +490,15 @@ mcrypto
>  Target Mask(CRYPTO) Var(rs6000_isa_flags)
>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
>  
>  mdirect-move
>  Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Enable direct move (ISA 2.07).

It is undocumented and should remain that, except eventually we should
remove it completely (but leave some stubs so that code in the wild
keeps compiling).

> +mpower8
> +Target Mask(POWER8) Var(rs6000_isa_flags)
> +Use instructions added in ISA 2.07 (power8).

There should not be such an option.  It is set by -mcpu=power8 and
later, but can never be enabled or disabled direfctly by the user.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
>    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
>      {
>        if (op0_regno == op1_regno)
>  	return ASM_COMMENT_START " vec_extract to same register";
>  
> -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
>  	       && TARGET_POWERPC64)

That fits on one line now.

Thanks,


Segher