public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: "Kewen.Lin" <linkw@linux.ibm.com>
To: Iain Sandoe <iain@sandoe.co.uk>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
	Segher Boessenkool <segher@kernel.crashing.org>
Subject: Re: [PATCH] rs6000: Rework option -mpowerpc64 handling [PR106680]
Date: Thu, 29 Sep 2022 13:45:16 +0800	[thread overview]
Message-ID: <5e64fed0-7e79-3d60-da62-5c2bf3e2c707@linux.ibm.com> (raw)
In-Reply-To: <92415AC8-4A5A-4119-BFCC-D7C66472F961@sandoe.co.uk>

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

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 <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:
>>>
>>>> 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,
Kewen

[-- Attachment #2: darwin.diff --]
[-- Type: text/plain, Size: 2941 bytes --]

diff --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)

  reply	other threads:[~2022-09-29  5:45 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-28  5:30 Kewen.Lin
2022-09-28  6:37 ` Iain Sandoe
2022-09-28 16:18   ` Iain Sandoe
2022-09-28 19:09     ` Iain Sandoe
2022-09-29  5:45       ` Kewen.Lin [this message]
2022-09-29  8:16         ` Iain Sandoe
2022-09-29  9:12           ` Kewen.Lin
2022-09-29 16:14             ` Iain Sandoe
2022-09-29 17:04           ` Segher Boessenkool
2022-09-29 18:25             ` Iain Sandoe
2022-09-29 18:37               ` Segher Boessenkool
2022-09-30  9:26                 ` Kewen.Lin
2022-09-29 17:11         ` Segher Boessenkool
2022-09-30 12:15           ` Kewen.Lin
2022-10-03 21:15             ` Segher Boessenkool
2022-10-10  2:15               ` Kewen.Lin
2022-10-10 13:58                 ` Segher Boessenkool
2022-10-12  8:26                   ` Kewen.Lin
2022-09-28 21:30     ` Segher Boessenkool
2022-09-28 23:04       ` Iain Sandoe
2022-09-28 23:16         ` Iain Sandoe
2022-09-29 17:26           ` Segher Boessenkool
2022-09-29 17:18         ` Segher Boessenkool
2022-09-29 18:33           ` Iain Sandoe
2022-09-29 18:50             ` Segher Boessenkool
2022-09-28 22:04 ` Segher Boessenkool
2022-09-29  6:16   ` Kewen.Lin
2022-09-29 18:56     ` Segher Boessenkool

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5e64fed0-7e79-3d60-da62-5c2bf3e2c707@linux.ibm.com \
    --to=linkw@linux.ibm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=iain@sandoe.co.uk \
    --cc=segher@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).