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.
>
> > + /* 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.
>
> > + /* 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.
>
> > +/* 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.
In the power8 days, we (mostly me) specifically spent a lot of time to add
fusion support. I ultimately ran out of time in the initial GCC release to do
all of the optimizations planned. Later, when it became apparent that power9
would not implement this fusion (origianlly it had been in the plan), the code
kind of withered, and it left some warts.
In particular, the fusion work was presented at the 2014 Gnu Cauldron (slides
24-27 in my deck) among the other power8 changes.
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.
>
> > +/* Power8 fusion does not fuse loads with sign extends. If we are not
> > + optimizing for space, split loads with sign extension to loads with zero
> > + extension and an explicit sign extend operation, so that the zero
> > extending
> > + load can be fused. */
> > +#define TARGET_P8_FUSION_SIGN (TARGET_P8_FUSION
> > \
> > + && !optimize_function_for_size_p (cfun))
>
> As I said before, don't do this. Just remove the whole thing.
>
> > +; The -mpower8-fusion and -mpower8-fusion-sign options existed in the
> > past, but
> > +; they are ignored now.
>
> Don't put them together. It is much easier for everything if they are
> separate, boring, and exactly like everything else.
>
> > mpower8-fusion
> > -Target Mask(P8_FUSION) Var(rs6000_isa_flags)
> > -Fuse certain integer operations together for better performance on power8.
> > +Target Undocumented Ignore
>
> It should be deleted, instead, and be replaced with
>
> ;; This option existed in the past, but now is always off.
> mno-power8-fusion
> Target RejectNegative Undocumented Ignore
>
> mpower8-fusion
> Target RejectNegative Undocumented WarnRemoved
>
> i.e. just like all other removed flags. If someone explicitly tries to
> enable it he/she *should* get a warning.
>
> > 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.
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meiss...@linux.ibm.com
