public inbox for
 help / color / mirror / Atom feed
From: Segher Boessenkool <>
To: will schmidt <>
Cc: GCC patches <>,
	David Edelsohn <>,
	"Kewen.Lin" <>,
	Michael Meissner <>
Subject: Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)
Date: Mon, 17 Oct 2022 13:08:40 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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
    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

> The existing (and rather lengthy) commentary for DIRECT_MOVE remains
> in place in  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_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/
> +++ b/gcc/config/rs6000/
> @@ -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.



  parent reply	other threads:[~2022-10-17 18:09 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-19 16:05 [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2) will schmidt
2022-09-19 16:13 ` [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2) will schmidt
2022-10-13 16:07   ` will schmidt
2022-10-17 12:55   ` Kewen.Lin
2022-10-17 18:08   ` Segher Boessenkool [this message]
2022-10-18 15:17     ` will schmidt
2022-10-18 16:52       ` Segher Boessenkool
2022-10-19  2:36         ` Kewen.Lin
2022-10-17 12:54 ` [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2) Kewen.Lin
2022-10-17 15:32 ` Segher Boessenkool
2022-10-17 16:54   ` will schmidt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).