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

[will schmidt <will_schmidt@vnet.ibm.com>]

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