public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: will schmidt <will_schmidt@vnet.ibm.com>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: GCC patches <gcc-patches@gcc.gnu.org>,
	David Edelsohn <dje.gcc@gmail.com>,
	"Kewen.Lin" <linkw@linux.ibm.com>,
	Michael Meissner <meissner@linux.vnet.ibm.com>
Subject: Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)
Date: Tue, 18 Oct 2022 10:17:30 -0500	[thread overview]
Message-ID: <f73c958c33ef2def7cf14450d4b0b8593de4d18c.camel@vnet.ibm.com> (raw)
In-Reply-To: <20221017180840.GJ25951@gate.crashing.org>

On Mon, 2022-10-17 at 13:08 -0500, Segher Boessenkool wrote:
> 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.

As long as it's OK to be removed, I'll certainly take a shot at it. 
With that in mind that may simplify things for me here.
I expect that
anything currently guarded by DIRECT_MOVE should instead be guarded by
POWER8.


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

fair enough.

> 
> > 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_PA
> > IR,
> >  								false,
> > true  },
> >    { "cmpb",			OPTION_MASK_CMPB,		fal
> > se, true  },
> >    { "crypto",			OPTION_MASK_CRYPTO,		fal
> > se, true  },
> >    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false,
> > true  },
> > +  { "power8",			OPTION_MASK_POWER8,		fal
> > se, true  },
> 
> Why would we want a #pragma power8 ?

Hmm, thinko on my part, i'll reevaluate.


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

OK.


Thanks for the detailed review.  :-)
-Will


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


  reply	other threads:[~2022-10-18 15:17 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
2022-10-18 15:17     ` will schmidt [this message]
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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=f73c958c33ef2def7cf14450d4b0b8593de4d18c.camel@vnet.ibm.com \
    --to=will_schmidt@vnet.ibm.com \
    --cc=dje.gcc@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=linkw@linux.ibm.com \
    --cc=meissner@linux.vnet.ibm.com \
    --cc=segher@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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).