Re: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059

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

On Thu, May 05, 2022 at 04:21:21PM -0400, Michael Meissner wrote:
> On Thu, May 05, 2022 at 02:12:43PM -0500, Segher Boessenkool wrote:
> > On Tue, Apr 12, 2022 at 09:14:55PM -0400, Michael Meissner wrote:
> > > This is V4 of the patch.  Compared to V3 of the patch, GCC will just
> > > ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.
> > 
> > But incorrectly :-(
> > 
> > > The splitting of signed halfword and word loads into unsigned load and
> > > sign extension is now suppressed with -Os, but it is done normally if we
> > > are not optimizing for space.
> > 
> > I have no idea what that means.  Other than that I asked to remove that.
> > 
> > > This code makes the -mpower8-fusion option a nop.  It is accepted without
> > > warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
> > > for a power8.
> > 
> > It should *delete* the option, and have
> > ;; This option existed in the past, but now is always off.
> > mno-power8-fusion
> > Target RejectNegative Undocumented Ignore
> > 
> > > The undocumented -mpower8-fusion-sign option is also made into a nop.
> > 
> > That one should be deleted.
> 
> Sure, but note the customer that asked for the patch, explicitly is using
> -mno-power8-fusion.  I don't want to break their makefiles.

-mno-power8-fusion is retained, and ignored.  -mpower8-fusion is
retained (but as separate option), ignored, and warned for.  Both
-mpower8-fusion-sign and -mno-power8-fusion-sign can be removed, since
nothing uses those options.

> > > +  /* The Power8 fusion option was removed.  We ignore using it in #pragma and
> > > +     attribute target.  Users may have used the options to suppress errors if
> > > +     they declare an inline function to be specifically power8 and the function
> > > +     was included by power9 or power10 which turned off the power8 fusion
> > > +     support.  */
> > > +  { "power8-fusion",		0,				false, true  },
> > 
> > What does the comment mean?
> 
> One of the suggestions that I made to the customer was to change their code
> from:
> 
> static inline int
> __attribute__ ((always_inline,target("cpu=power8")))
> foo (..)
> {
>   ...
> }
> 
> to:
> 
> static inline int
> __attribute__ ((always_inline,target("cpu=power8,no-power8-fusion")))
> foo (...)
> {
>   ...
> }
> 
> If they used this, it would avoid the issue, and not need to use a new switch.
> Whether they used it, I don't know.  Whether some other customer who ran into
> the problem used it, again I don't know.  If we remove the support for it in
> target pragma and attribute, we can break code.

There is still a -mno-power8-fusion option after you implemented what I
suggested all of this time.  This code will still just work, no?

> > > +      /* Don't print options that exist for backwards compatibility, but are
> > > +	 ignored now like -mpower8-fusion.  */
> > > +      if (!mask)
> > > +	continue;
> > 
> > No.  Such options should not be in the mask at all.
> 
> It is just to support ignoring setting no-power8-fusion in the target pragma
> and attribute options.

If you need to do that separately, there is something else wrong.  And
even then, if you need to do it manually, you should *do* it manually,
not make all kinds of coplications all over the place.

> > > +/* Power8 has special fusion operations that are enabled if we are tuning for
> > > +   power8.  This used to be settable with an option (-mpower8-fusion), but that
> > > +   option has been removed.  */
> > > +#define TARGET_P8_FUSION	(rs6000_tune == PROCESSOR_POWER8)
> > 
> > The plan was to not have p8 fusion at all.  GCC never implemented any of
> > the more useful p8 fusion things anyway, and those were only marginally
> > beneficial anyway.
> 
> No, no, no, no.  That is incorrect.

It was the plan I agreed with.

> And by the way, we likely will have a similar issue with power10 fusion.  I
> specifically have not done anything for that to avoid clouding the issue for
> this bug.  But I imagine we may need to look at that in the future.

Whether anything is fused or not should never be of any influence on
what is inlined or not in what or whatnot.  Fusion is comparable to
core-specific scheduling in almost all ways.

> > >  mpower8-fusion-sign
> > > -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> > > -Allow sign extension in fusion operations.
> > > +Target Undocumented Ignore
> > 
> > And this one should be completely removed, since no one ever used it.
> 
> Well like all tuning flags, it was used quite a lot when I was doing the
> initial work.

But it should arguably never have made it into any release branch.


Segher