On Thu, Apr 07, 2022 at 07:59:27PM -0400, Michael Meissner wrote:
> I have tested this patch on a little endian power10 system. I have tested
> previous versions on little endian power9 and big endian power8 systems.
Please test on at least p8 as well.
> I will want to backport the patch to GCC 10 and GCC 11 in a few days.
> Note this patch can't be used directly for those backports due to the
> other changes for PR target/102059 that have gone into the master branch
> but were not back ported.
Then send a new patch for there. If you cannot backport, you cannot
backport.
> (rs6000_opt_masks): Allow #pragma target and attribute target to set
> power8-fusion, but these no longer represent an option that the
> user can set.
Do you have evidence that anything used this in the wild? Nothing
should have. Please try to remove this in GCC 13 at least.
> (rs6000_print_options_internal): Skip printing nop options.
It is not clear what you mean here (not in the code either). Please
rephrase.
> * config/rs6000/rs6000.opt (-mpower8-fusion): Recognize the option but
> ignore the no form and warn that the option was removed for the regular
> form.
If it is ignored, you should not warn for it.
> (-mpower8-fusion-sign): Warn that the option has been removed.
And no one could correctly have used this ever, so don't warn anything
about this in any case.
> -/* For now, don't provide an embedded version of ISA 2.07. Do not set power8
> - fusion here, instead set it in rs6000.cc if we are tuning for a power8
> - system. */
You should keep part of this comment. Maybe just "We don't implement an
embedded version of ISA 2.07 ."?
> + /* 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 },
This sounds very hypothetical. Does this actually happen? If so,
rephrase the comment; if not, remove the whole thing.
> @@ -24687,6 +24657,10 @@ rs6000_print_options_internal (FILE *file,
> HOST_WIDE_INT mask = opts[i].mask;
> size_t len = comma_len + prefix_len + strlen (name);
>
> + /* Don't print NOP options. */
> + if (!mask)
> + continue;
It is not clear at all what "NOP options" means. I can read the code
to understand the comment, but that makes the code less readable than
without comment. Please fix this.
> +/* 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)
That comment does not belong here.
> +/* Power8 fusion does not fuse loads with sign extends. If we are doing
> higher
> + optimization levels, 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_speed_p (cfun) \
> + && optimize >= 3)
This should not depend on -O3 or sneaky ways to disable optimising for
speed. It does not have to anyway. Just remove this whole thing, use
TARGET_P8_FUSION instead everywhere?
> +; The -mpower8-fusion option existed in the past, but it has been removed.
> +; Some users in the past needed to use the -mno-power8-fusion option when
> they
> +; had inline functions that were specified as generating power8 code and the
> +; functions were included by power9 or power10 function. Using
> +; -mno-power8-fusion prevented an error in this case. We allow the
> +; -mno-power8-fusion option without a warning.
Please don't say all these things. It should say what all our other
removed options say: a single half-line comment, and nothing more.
> mpower8-fusion
> -Target Mask(P8_FUSION) Var(rs6000_isa_flags)
> -Fuse certain integer operations together for better performance on power8.
> +Target RejectNegative Undocumented WarnRemoved
> +mno-power8-fusion
> +Target RejectNegative Undocumented
It also should have "Ignore".
The "no-" variant (the always active one!) goes first. Both share that
one short comment.
> +; The -mpower8-fusion-sign option was an undocumented option that modified
> the
> +; -mpower8-fusion option. It has been removed.
> mpower8-fusion-sign
> -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> -Allow sign extension in fusion operations.
> +Target Undocumented WarnRemoved
Don't WarnRemoved. Do Ignore. Just remove the whole thing please.
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> @@ -0,0 +1,23 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +/* { dg-require-effective-target power10_ok } */
These last two lines the other way around: first state requirements, and
only then the options that those are for.
Please resend with those things fixed. Thanks!
Segher
