Re: [PATCH] rs6000: Rework option -mpowerpc64 handling [PR106680]

[Iain Sandoe <iain@sandoe.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]

Hi Kewen,

thanks for looking at this!
(I suspect it would also affect a 32b linux host with a 64b multilib)

quite likely powerpc-darwin is the only 32b ppc host in regular testing.

> On 29 Sep 2022, at 06:45, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> on 2022/9/29 03:09, Iain Sandoe wrote:
>> Hi Kewen
>> 
>>> On 28 Sep 2022, at 17:18, Iain Sandoe <iain@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 <iain@sandoe.co.uk> wrote:
>>> 
>>>>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>> 

>>> ------
>>> 
>>> /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?

It does work on a cross from x86_64-darwin => powerpc-darwin, I can also do compile-only
tests there with a dummy board and the new tests pass with one minor tweak as described
below.

full regstrap on the G5 will take a day or so .. but I’ll do the C target tests first to get a heads up.

====

OK. So one small wrinkle, 

Darwin already has 

  if (TARGET_64BIT && ! TARGET_POWERPC64)
    {
      rs6000_isa_flags |= OPTION_MASK_POWERPC64;
      warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64");
    }

in darwin_rs6000_override_options()

Which means that we do not report an error, but a warning, and then we force 64b on (taking
the user’s intention to be specified by the explicit ‘-m64’).

If there’s a strong feeling that this should really be an error, then I could make that change and
see what fallout results.

the patch below amends the test expectations to include Darwin with the warning it currently
reports.

cheers
Iain


[-- Attachment #2: darwin-tests.diff --]
[-- Type: application/octet-stream, Size: 1895 bytes --]

diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-1.c b/gcc/testsuite/gcc.target/powerpc/pr106680-1.c
index ff33f6864c3..55424f18a78 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr106680-1.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr106680-1.c
@@ -9,4 +9,5 @@ int foo ()
 }
 
 /* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */
+/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
 /* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-2.c b/gcc/testsuite/gcc.target/powerpc/pr106680-2.c
index 25439910c27..ae0d6afca38 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr106680-2.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr106680-2.c
@@ -10,4 +10,5 @@ int foo ()
 }
 
 /* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */
+/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
 /* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-3.c b/gcc/testsuite/gcc.target/powerpc/pr106680-3.c
index f8eea8ea645..702b2c00a34 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr106680-3.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr106680-3.c
@@ -9,4 +9,5 @@ int foo ()
 }
 
 /* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */
+/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
 /* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */

[-- Attachment #3: Type: text/plain, Size: 4 bytes --]