Hi Iain,
Thanks very much for your time!!!
on 2022/9/29 03:09, Iain Sandoe wrote:
> Hi Kewen
>
>> On 28 Sep 2022, at 17:18, Iain Sandoe <i...@sandoe.co.uk> wrote:
>>
>> (reduced CC list, if folks want to be re-included .. please add them back).
>>
>>> On 28 Sep 2022, at 07:37, Iain Sandoe <i...@sandoe.co.uk> wrote:
>>
>>>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches
>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>> PR106680 shows that -m32 -mpowerpc64 is different from
>>>> -mpowerpc64 -m32, this is determined by the way how we
>>>> handle option powerpc64 in rs6000_handle_option.
>>>>
>>>> Segher pointed out this difference should be taken as
>>>> a bug and we should ensure that option powerpc64 is
>>>> independent of -m32/-m64. So this patch removes the
>>>> handlings in rs6000_handle_option and add some necessary
>>>> supports in rs6000_option_override_internal instead.
>>>>
>>>> With this patch, if users specify -m{no-,}powerpc64, the
>>>> specified value is honoured, otherwise, for 64bit it
>>>> always enables OPTION_MASK_POWERPC64 while for 32bit
>>>> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64.
>>>>
>>>> Bootstrapped and regress-tested on:
>>>> - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>>>> - powerpc64le-linux-gnu P9 and P10
>>>> - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
>>>>
>>>> Hi Iain, could you help to test this on darwin to ensure
>>>> it won't break darwin's build and new tests are fine?
>>>> Thanks in advance!
>>>
>>> Will do, it will take a day or so, thanks,
>>
>> Perhaps a small exposition on the target:
>>
>> powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a
>> 32b kernel, but the OS supports 64b processes on suitable hardware (and the
>> OS does preserve the upper bits of 64b regs in the context).
>>
>> -----
>>
>> I bootstrapped (all supported languages) and tested r13-2892 yesterday with
>> “nominal” results.
>>
>> Then I added this patch .. and did a clean bootstrap (same configuration).
>>
>> the bootstrap fails on the stage3 libgomp (building the ppc64 multilib) with
>> the error below
>> What is somewhat odd here is that libgomp is bootstrapped with the compiler
>> and, apparently,
>> openacc-init.c built OK at stage2.
>>
>> ——
>>
>> Of course, powerpc-darwin is not a blocker for anything, it should not hold
>> you up (but sometimes it
>> manages to find a glitch missed elsewhere). I will try to take a look at
>> this this evening see if I can throw
>> any more light on it.
>>
>> ------
>>
>> /src-local/gcc-master/libgomp/oacc-init.c:876:1: internal compiler error:
>> ‘global_options’ are modified in local context
>> 876 | {
>> | ^
>> 0xe940d7 cl_optimization_compare(gcc_options*, gcc_options*)
>> /scratch/10-5-leo/gcc-master/gcc/options-save.cc:14082
>
> This repeats on a cross from x86_64-darwin to powerpc-darwin .. (makes debug
> a bit quicker)
>
> this is the failing case - which does not (immediately) seem directly
> connected .. does it ring
> any bells for you?
>
> 16649 if (ptr1->x_rs6000_sched_restricted_insns_priority !=
> ptr2->x_rs6000_sched_restricted_insns_priority)
> -> 16650 internal_error ("%<global_options%> are modified in local
> context”);
>
I found this flag is mainly related to tune setting and spotted that we have
some code
for tune setting when no explicit cpu is given.
...
else
{
size_t i;
enum processor_type tune_proc
= (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT);
tune_index = -1;
for (i = 0; i < ARRAY_SIZE (processor_target_table); i++)
if (processor_target_table[i].processor == tune_proc)
{
tune_index = i;
break;
}
}
It checks TARGET_POWERPC64 directly here, my proposed patch will adjust
TARGET_POWERPC64
after this hunk, so it seems to be problematic for some case.
I'm testing the attached diff which can be applied on top of the previous
proposed patch
on ppc64 and ppc64le, could you help to test it can fix the issue?
BR,
Kewendiff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 605d35893f9..3bfbb4eac21 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3702,7 +3702,7 @@ rs6000_option_override_internal (bool global_init_p)
else
{
/* PowerPC 64-bit LE requires at least ISA 2.07. */
- const char *default_cpu = (!TARGET_POWERPC64
+ const char *default_cpu = (!TARGET_POWERPC64 && TARGET_32BIT
? "powerpc"
: (BYTES_BIG_ENDIAN
? "powerpc64"
@@ -3713,6 +3713,26 @@ rs6000_option_override_internal (bool global_init_p)
rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit);
}
+ /* With option powerpc64 specified explicitly (either on or off), even if
+ being compiled for 64 bit we don't need to check if it's disabled here,
+ since subtargets will check and raise an error message if necessary
+ later. But without option powerpc64 specified explicitly, we need to
+ ensure powerpc64 enabled for 64 bit and disabled on those OSes with
+ OS_MISSING_POWERPC64, since they don't support saving the high part of
+ 64-bit registers on context switch. */
+ if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
+ {
+ if (TARGET_64BIT)
+ /* Make sure we always enable it by default for 64 bit. */
+ rs6000_isa_flags |= OPTION_MASK_POWERPC64;
+#ifdef OS_MISSING_POWERPC64
+ else if (OS_MISSING_POWERPC64)
+ /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which
+ miss powerpc64 support, so disable it. */
+ rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
+#endif
+ }
+
if (rs6000_tune_index >= 0)
tune_index = rs6000_tune_index;
else if (cpu_index >= 0)
@@ -3748,26 +3768,6 @@ rs6000_option_override_internal (bool global_init_p)
error ("AltiVec not supported in this target");
}
- /* With option powerpc64 specified explicitly (either on or off), even if
- being compiled for 64 bit we don't need to check if it's disabled here,
- since subtargets will check and raise an error message if necessary
- later. But without option powerpc64 specified explicitly, we need to
- ensure powerpc64 enabled for 64 bit and disabled on those OSes with
- OS_MISSING_POWERPC64, since they don't support saving the high part of
- 64-bit registers on context switch. */
- if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
- {
- if (TARGET_64BIT)
- /* Make sure we always enable it by default for 64 bit. */
- rs6000_isa_flags |= OPTION_MASK_POWERPC64;
-#ifdef OS_MISSING_POWERPC64
- else if (OS_MISSING_POWERPC64)
- /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which
- miss powerpc64 support, so disable it. */
- rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
-#endif
- }
-
/* If we are optimizing big endian systems for space, use the load/store
multiple instructions. */
if (BYTES_BIG_ENDIAN && optimize_size)
