Hi Mike,
On 16/07/25 7:37 am, Michael Meissner wrote:
> On Tue, Jul 15, 2025 at 08:11:05AM -0500, Segher Boessenkool wrote:
>> Hi!
>>
>> On Tue, Jul 01, 2025 at 12:14:32PM -0400, Michael Meissner wrote:
>>> This patch adds the support that can be used in developing GCC support
>>> for potential future PowerPC processors.
>>>
>>> These changes are being added so that hardware designers can evaluate
>>> potential new features to be added to the PowerPC processors in the
>>> future. It may be these features will be incorporated into real
>>> hardware using a different name in the future. Or it may be these
>>> features will not be incoporated into actual PowerPC hardware in the
>>> future.
>>
>> None of this belongs in the commit message. In the furure please post
>> stuff exactly as you want it to be committed!
>
> The patch includes comments regarding previous patches, and aren't the
> commit patch. If I eliminate those comments, you will then say things
> like 'where did this come from'.
>
> I can try to separate things so there is the initial commentary meant
> for the review to address concerns from previous patches.
>
>>> * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): If
>>> -mcpu=future, define _ARCH_FUTURE.
>>> * config/rs6000/rs6000-cpus.def (FUTURE_MASKS_SERVER): New macro.
>>> (POWERPC_MASKS): Add OPTION_MASK_FUTURE.
>>> (future cpu): Add 'future' cpu.
>>> * config/rs6000/rs6000-tables.opt: Regenerate.
>>> * config/rs6000/rs6000.opt (-mfuture-internal): New internal option to
>>> indicate the user used -mcpu=future.
>>
>> NAK, as said before.
>>
>> If the user wants to see what rs6000_cpu is set to, the user should just
>> look at rs6000_cpu, don't go via more indirect roundabout ways.
>>
>>> +/* At present, we are not providing a unique processor option for
>>> -mcpu=future.
>>
>> What does this comment mean even?
>
> We have essentially two choices, and I feel you have not liked either.
> I don't see a clear way forward.
>
> The way I did it before was to have a separate processor enumeration
> for cpu=future. This involves adding switch statements and such for
> every place we test for cpu == power10 or power11. And it also means
> that power10.md has lots of edits to add the future case.
>
> Or we can do it the way I did in this patch, to not create a new cpu
> enumeration at the present time, and just set it to power11. Some day
> in the future, sure we will move things around, but at the current
> time, we are not adding unique scheduling for cpu=future.
This approach is fine.
- Please modify the commit message to only specify what is being committed
in this patch.
- Please also remove -mfuture-internal.
- Please reword the comment for the following change:
+ RS6000_CPU ("future", PROCESSOR_POWER11, MASK_POWERPC64 | FUTURE_MASKS_SERVER)
perhaps something like:
"For now, use the power11 defaults for the 'future' processor"...
Regards
Surya
>
> It really doesn't matter to me, as I've done both approaches. Do you
> want me to go back to having a separate future cpu enumeration, which
> involves all of the changes we've seen in the past, or did you want the
> more minimal changes that I just did.
>
>>> + Until we define these changes, just use the power11 defaults. */
>>> +RS6000_CPU ("future", PROCESSOR_POWER11, MASK_POWERPC64 |
>>> FUTURE_MASKS_SERVER)
>>
>> It isn't talking about this code for sure.
>>
>>> +;; Users should not use -mfuture-internal, but we need to use a bit to
>>> identify
>>> +;; when the user changes the default cpu via #pragma GCC
>>> target("cpu=future")
>>> +;; and then resets it later.
>>
>> What does that mean? It hints at a bug elsewhere, but not more than
>> hints unfortunately.
>>
>>
>> Segher
>
