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
