Hello, On Wed, Dec 11 2024, Martin Jambor wrote: > Hello, > > even though it is not my work, I would like to ping this patch. Having > it upstream would really help us a lot. >
Please, pretty please, consider reviewing this in time for GCC 15, having it upstream would really help us a lot and from what I can tell, it should no longer be controversial. Thank you very much in advance, Martin > > On Wed, Nov 13 2024, Michael Matz wrote: >> Hello, >> >> this is essentially >> >> https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651025.html >> >> from Kewen in functionality. When discussing this with Segher at the >> Cauldron he expressed reservations about changing the default >> implementation of -fpatchable-function-entry. So, to move forward, let's >> move it under a new target option -msplit-patch-nops (expressing the >> important deviation from the default behaviour, namely that all the >> patching nops form a consecutive sequence normally). >> >> Regstrapping on power9 ppc64le in progress. Okay if that passes? >> >> >> Ciao, >> Michael. >> >> --- >> >> as the bug report details some uses of -fpatchable-function-entry >> aren't happy with the "before" NOPs being inserted between global and >> local entry point on powerpc. We want the before NOPs be in front >> of the global entry point. That means that the patching NOPs aren't >> consecutive for dual entry point functions, but for these usecases >> that's not the problem. But let us support both under the control >> of a new target option: -msplit-patch-nops. >> >> gcc/ >> >> PR target/112980 >> * config/rs6000/rs6000.opt (msplit-patch-nops): New option. >> * doc/invoke.texi (RS/6000 and PowerPC Options): Document it. >> * config/rs6000/rs6000.h (machine_function.stop_patch_area_print): >> New member. >> * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry): >> Emit split nops under control of that one. >> * config/rs6000/rs6000-logue.cc (rs6000_output_function_prologue): >> Add handling of split patch nops. >> --- >> gcc/config/rs6000/rs6000-logue.cc | 15 +++++++++------ >> gcc/config/rs6000/rs6000.cc | 27 +++++++++++++++++++++++---- >> gcc/config/rs6000/rs6000.h | 6 ++++++ >> gcc/config/rs6000/rs6000.opt | 4 ++++ >> gcc/doc/invoke.texi | 17 +++++++++++++++-- >> 5 files changed, 57 insertions(+), 12 deletions(-) >> >> diff --git a/gcc/config/rs6000/rs6000-logue.cc >> b/gcc/config/rs6000/rs6000-logue.cc >> index c87058b435e..aa1e0442f2b 100644 >> --- a/gcc/config/rs6000/rs6000-logue.cc >> +++ b/gcc/config/rs6000/rs6000-logue.cc >> @@ -4005,8 +4005,8 @@ rs6000_output_function_prologue (FILE *file) >> >> unsigned short patch_area_size = crtl->patch_area_size; >> unsigned short patch_area_entry = crtl->patch_area_entry; >> - /* Need to emit the patching area. */ >> - if (patch_area_size > 0) >> + /* Emit non-split patching area now. */ >> + if (!TARGET_SPLIT_PATCH_NOPS && patch_area_size > 0) >> { >> cfun->machine->global_entry_emitted = true; >> /* As ELFv2 ABI shows, the allowable bytes between the global >> @@ -4027,7 +4027,6 @@ rs6000_output_function_prologue (FILE *file) >> patch_area_entry); >> rs6000_print_patchable_function_entry (file, patch_area_entry, >> true); >> - patch_area_size -= patch_area_entry; >> } >> } >> >> @@ -4037,9 +4036,13 @@ rs6000_output_function_prologue (FILE *file) >> assemble_name (file, name); >> fputs ("\n", file); >> /* Emit the nops after local entry. */ >> - if (patch_area_size > 0) >> - rs6000_print_patchable_function_entry (file, patch_area_size, >> - patch_area_entry == 0); >> + if (patch_area_size > patch_area_entry) >> + { >> + patch_area_size -= patch_area_entry; >> + cfun->machine->stop_patch_area_print = false; >> + rs6000_print_patchable_function_entry (file, patch_area_size, >> + patch_area_entry == 0); >> + } >> } >> >> else if (rs6000_pcrel_p ()) >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> index 950fd947fda..6427e6913ba 100644 >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -15226,11 +15226,25 @@ rs6000_print_patchable_function_entry (FILE *file, >> { >> bool global_entry_needed_p = rs6000_global_entry_point_prologue_needed_p >> (); >> /* For a function which needs global entry point, we will emit the >> - patchable area before and after local entry point under the control of >> - cfun->machine->global_entry_emitted, see the handling in function >> - rs6000_output_function_prologue. */ >> - if (!global_entry_needed_p || cfun->machine->global_entry_emitted) >> + patchable area when it isn't split before and after local entry point >> + under the control of cfun->machine->global_entry_emitted, see the >> + handling in function rs6000_output_function_prologue. */ >> + if (!TARGET_SPLIT_PATCH_NOPS >> + && (!global_entry_needed_p || cfun->machine->global_entry_emitted)) >> default_print_patchable_function_entry (file, patch_area_size, >> record_p); >> + >> + /* For split patch nops we emit the before nops (from generic code) >> + in front of the global entry point and after the local entry point, >> + under the control of cfun->machine->stop_patch_area_print, see >> + rs6000_output_function_prologue and rs6000_elf_declare_function_name. >> */ >> + if (TARGET_SPLIT_PATCH_NOPS) >> + { >> + if (!cfun->machine->stop_patch_area_print) >> + default_print_patchable_function_entry (file, patch_area_size, >> + record_p); >> + else >> + gcc_assert (global_entry_needed_p); >> + } >> } >> >> enum rtx_code >> @@ -21423,6 +21437,11 @@ rs6000_elf_declare_function_name (FILE *file, const >> char *name, tree decl) >> fprintf (file, "\t.previous\n"); >> } >> ASM_OUTPUT_FUNCTION_LABEL (file, name, decl); >> + /* At this time, the "before" NOPs have been already emitted. >> + For split nops stop generic code from printing the "after" NOPs and >> + emit them just after local entry ourself later. */ >> + if (rs6000_global_entry_point_prologue_needed_p ()) >> + cfun->machine->stop_patch_area_print = true; >> } >> >> static void rs6000_elf_file_end (void) ATTRIBUTE_UNUSED; >> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h >> index d460eb06544..2bd56f8d377 100644 >> --- a/gcc/config/rs6000/rs6000.h >> +++ b/gcc/config/rs6000/rs6000.h >> @@ -2424,6 +2424,12 @@ typedef struct GTY(()) machine_function >> global entry. It helps to control the patchable area before and after >> local entry. */ >> bool global_entry_emitted; >> + /* With ELFv2 ABI dual entry points being adopted, generic framework >> + targetm.asm_out.print_patchable_function_entry would generate "after" >> + NOPs before local entry, which is wrong. This flag is to stop it from >> + printing patch area before local entry, it is only useful when the >> + function requires dual entry points. */ >> + bool stop_patch_area_print; >> } machine_function; >> #endif >> >> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt >> index 94323bd1db2..27a165ef0ee 100644 >> --- a/gcc/config/rs6000/rs6000.opt >> +++ b/gcc/config/rs6000/rs6000.opt >> @@ -300,6 +300,10 @@ mfull-toc >> Target >> Put everything in the regular TOC. >> >> +msplit-patch-nops >> +Target Var(TARGET_SPLIT_PATCH_NOPS) Init(0) >> +Emit NOPs before global and after local entry point for >> -fpatchable-function-entry. >> + >> mvrsave >> Target Var(TARGET_ALTIVEC_VRSAVE) Save >> Generate VRSAVE instructions when generating AltiVec code. >> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >> index 93e9af3791c..f3d37a48fd0 100644 >> --- a/gcc/doc/invoke.texi >> +++ b/gcc/doc/invoke.texi >> @@ -1318,6 +1318,7 @@ See RS/6000 and PowerPC Options. >> -mtraceback=@var{traceback_type} >> -maix-struct-return -msvr4-struct-return >> -mabi=@var{abi-type} -msecure-plt -mbss-plt >> +-msplit-patch-nops >> -mlongcall -mno-longcall -mpltseq -mno-pltseq >> -mblock-move-inline-limit=@var{num} >> -mblock-compare-inline-limit=@var{num} >> @@ -18569,11 +18570,12 @@ If @code{N=0}, no pad location is recorded. >> The NOP instructions are inserted at---and maybe before, depending on >> @var{M}---the function entry address, even before the prologue. On >> PowerPC with the ELFv2 ABI, for a function with dual entry points, >> -the local entry point is this function entry address. >> +the local entry point is this function entry address by default. See >> +the @option{-msplit-patch-nops} option to change this. >> >> The maximum value of @var{N} and @var{M} is 65535. On PowerPC with the >> ELFv2 ABI, for a function with dual entry points, the supported values >> -for @var{M} are 0, 2, 6 and 14. >> +for @var{M} are 0, 2, 6 and 14 when not using @option{-msplit-patch-nops}. >> @end table >> >> >> @@ -31658,6 +31660,17 @@ requires @code{.plt} and @code{.got} >> sections that are both writable and executable. >> This is a PowerPC 32-bit SYSV ABI option. >> >> +@opindex msplit-patch-nops >> +@item -msplit-patch-nops >> +When adding NOPs for a patchable area via the >> +@option{-fpatchable-function-entry} option emit the "before" NOPs in front >> +of the global entry point and the "after" NOPs after the local entry point. >> +This makes the sequence of NOPs not consecutive when a global entry point >> +is generated. Without this option the NOPs are emitted directly before and >> +after the local entry point, making them consecutive but moving global and >> +local entry point further apart. If only a single entry point is generated >> +this option has no effect. >> + >> @opindex misel >> @opindex mno-isel >> @item -misel >> -- >> 2.35.3
