Hi, Gentle ping this patch:
https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651025.html BR, Kewen >> on 2024/7/12 00:15, Martin Jambor wrote: >>> Hi, >>> >>> can I add myself to the bunch of people who are pinging this? >>> Having >>> this in will make our life easier. >>> >>> Thanks a lot, >>> >>> Martin >>> >>> >>> On Wed, May 08 2024, Kewen.Lin wrote: >>>> Hi, >>>> >>>> As the discussion in PR112980, although the current >>>> implementation for -fpatchable-function-entry* conforms >>>> with the documentation (making N NOPs be consecutive), >>>> it's inefficient for both kernel and userspace livepatching >>>> (see comments in PR for the details). >>>> >>>> So this patch is to change the current implementation by >>>> emitting the "before" NOPs before global entry point and >>>> the "after" NOPs after local entry point. The new behavior >>>> would not keep NOPs to be consecutive, so the documentation >>>> is updated to emphasize this. >>>> >>>> Bootstrapped and regress-tested on powerpc64-linux-gnu >>>> P8/P9 and powerpc64le-linux-gnu P9 and P10. >>>> >>>> Is it ok for trunk? And backporting to active branches >>>> after burn-in time? I guess we should also mention this >>>> change in changes.html? >>>> >>>> BR, >>>> Kewen >>>> ----- >>>> PR target/112980 >>>> >>>> gcc/ChangeLog: >>>> >>>> * config/rs6000/rs6000-logue.cc >>>> (rs6000_output_function_prologue): >>>> Adjust the handling on patch area emitting with dual >>>> entry, remove >>>> the restriction on "before" NOPs count, not emit >>>> "before" NOPs any >>>> more but only emit "after" NOPs. >>>> * config/rs6000/rs6000.cc >>>> (rs6000_print_patchable_function_entry): >>>> Adjust by respecting cfun->machine- >>>>> stop_patch_area_print. >>>> (rs6000_elf_declare_function_name): For ELFv2 with dual >>>> entry, set >>>> cfun->machine->stop_patch_area_print as true. >>>> * config/rs6000/rs6000.h (struct machine_function): >>>> Remove member >>>> global_entry_emitted, add new member >>>> stop_patch_area_print. >>>> * doc/invoke.texi (option -fpatchable-function-entry): >>>> Adjust the >>>> documentation for PowerPC ELFv2 dual entry. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * c-c++-common/patchable_function_entry-default.c: >>>> Adjust. >>>> * gcc.target/powerpc/pr99888-4.c: Likewise. >>>> * gcc.target/powerpc/pr99888-5.c: Likewise. >>>> * gcc.target/powerpc/pr99888-6.c: Likewise. >>>> --- >>>> gcc/config/rs6000/rs6000-logue.cc | 40 +++++-------- >>>> ------ >>>> gcc/config/rs6000/rs6000.cc | 15 +++++-- >>>> gcc/config/rs6000/rs6000.h | 10 +++-- >>>> gcc/doc/invoke.texi | 8 ++-- >>>> .../patchable_function_entry-default.c | 3 -- >>>> gcc/testsuite/gcc.target/powerpc/pr99888-4.c | 4 +- >>>> gcc/testsuite/gcc.target/powerpc/pr99888-5.c | 4 +- >>>> gcc/testsuite/gcc.target/powerpc/pr99888-6.c | 4 +- >>>> 8 files changed, 33 insertions(+), 55 deletions(-) >>>> >>>> diff --git a/gcc/config/rs6000/rs6000-logue.cc >>>> b/gcc/config/rs6000/rs6000-logue.cc >>>> index 60ba15a8bc3..0eb019b44b3 100644 >>>> --- a/gcc/config/rs6000/rs6000-logue.cc >>>> +++ b/gcc/config/rs6000/rs6000-logue.cc >>>> @@ -4006,43 +4006,21 @@ rs6000_output_function_prologue (FILE >>>> *file) >>>> fprintf (file, "\tadd 2,2,12\n"); >>>> } >>>> >>>> - 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) >>>> - { >>>> - cfun->machine->global_entry_emitted = true; >>>> - /* As ELFv2 ABI shows, the allowable bytes between the >>>> global >>>> - and local entry points are 0, 4, 8, 16, 32 and 64 >>>> when >>>> - there is a local entry point. Considering there >>>> are two >>>> - non-prefixed instructions for global entry point >>>> prologue >>>> - (8 bytes), the count for patchable nops before >>>> local entry >>>> - point would be 2, 6 and 14. It's possible to >>>> support those >>>> - other counts of nops by not making a local entry >>>> point, but >>>> - we don't have clear use cases for them, so leave >>>> them >>>> - unsupported for now. */ >>>> - if (patch_area_entry > 0) >>>> - { >>>> - if (patch_area_entry != 2 >>>> - && patch_area_entry != 6 >>>> - && patch_area_entry != 14) >>>> - error ("unsupported number of nops before >>>> function entry (%u)", >>>> - patch_area_entry); >>>> - rs6000_print_patchable_function_entry (file, >>>> patch_area_entry, >>>> - true); >>>> - patch_area_size -= patch_area_entry; >>>> - } >>>> - } >>>> - >>>> fputs ("\t.localentry\t", file); >>>> assemble_name (file, name); >>>> fputs (",.-", 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); >>>> + unsigned short patch_area_size = crtl->patch_area_size; >>>> + unsigned short patch_area_entry = crtl->patch_area_entry; >>>> + if (patch_area_size > patch_area_entry) >>>> + { >>>> + cfun->machine->stop_patch_area_print = false; >>>> + patch_area_size -= patch_area_entry; >>>> + 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 6ba9df4f02e..7e70741d59f 100644 >>>> --- a/gcc/config/rs6000/rs6000.cc >>>> +++ b/gcc/config/rs6000/rs6000.cc >>>> @@ -15190,12 +15190,14 @@ rs6000_print_patchable_function_entry >>>> (FILE *file, >>>> bool record_p) >>>> { >>>> 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 >>>> + /* For a function which needs global entry point, we will only >>>> emit the >>>> + patchable area after local entry point under the control of >>>> + !cfun->machine->stop_patch_area_print, see the handling in >>>> functions >>>> rs6000_output_function_prologue. */ >>>> - if (!global_entry_needed_p || cfun->machine- >>>>> global_entry_emitted) >>>> + 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 >>>> @@ -21381,6 +21383,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, >>>> + let's stop generic code from printing the "after" NOPs and >>>> + emit just after local entry 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 68bc45d65ba..73767fdae06 100644 >>>> --- a/gcc/config/rs6000/rs6000.h >>>> +++ b/gcc/config/rs6000/rs6000.h >>>> @@ -2434,10 +2434,12 @@ typedef struct GTY(()) machine_function >>>> bool lr_is_wrapped_separately; >>>> bool toc_is_wrapped_separately; >>>> bool mma_return_type_error; >>>> - /* Indicate global entry is emitted, only useful when the >>>> function requires >>>> - 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, it 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/doc/invoke.texi b/gcc/doc/invoke.texi >>>> index c584664e168..58e48f7dc55 100644 >>>> --- a/gcc/doc/invoke.texi >>>> +++ b/gcc/doc/invoke.texi >>>> @@ -18363,11 +18363,11 @@ 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. >>>> +@var{M} NOP instructions are inserted before the global entry >>>> point and >>>> +@var{N} - @var{M} NOP instructions are inserted after the local >>>> entry >>>> +point, which means the NOP instructions may not be consecutive. >>>> >>>> -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. >>>> +The maximum value of @var{N} and @var{M} is 65535. >>>> @end table >>>> >>>> >>>> diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry- >>>> default.c b/gcc/testsuite/c-c++-common/patchable_function_entry- >>>> default.c >>>> index 3ccbafc87db..899938b4aa3 100644 >>>> --- a/gcc/testsuite/c-c++-common/patchable_function_entry- >>>> default.c >>>> +++ b/gcc/testsuite/c-c++-common/patchable_function_entry- >>>> default.c >>>> @@ -1,9 +1,6 @@ >>>> /* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } >>>> */ >>>> /* { dg-options "-O2 -fpatchable-function-entry=3,1" } */ >>>> /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ >>>> -/* See PR99888, one single preceding nop isn't allowed on >>>> powerpc_elfv2, >>>> - so overriding with two preceding nops to make it pass there. >>>> */ >>>> -/* { dg-additional-options "-fpatchable-function-entry=3,2" { >>>> target powerpc_elfv2 } } */ >>>> /* { dg-final { scan-assembler-times "nop|NOP|SWYM" 3 { target { >>>> ! { alpha*-*-* riscv*-*-* } } } } } */ >>>> /* { dg-final { scan-assembler-times "bis" 3 { target alpha*-*-* >>>> } } } */ >>>> /* { dg-final { scan-assembler-times "nop\n" 3 { target riscv*- >>>> *-* } } } */ >>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr99888-4.c >>>> b/gcc/testsuite/gcc.target/powerpc/pr99888-4.c >>>> index 00a8d4d316e..6f23f2bb939 100644 >>>> --- a/gcc/testsuite/gcc.target/powerpc/pr99888-4.c >>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-4.c >>>> @@ -2,12 +2,10 @@ >>>> /* There is no global entry point prologue with pcrel. */ >>>> /* { dg-options "-mno-pcrel -fpatchable-function-entry=1,1" } */ >>>> >>>> -/* Verify one error emitted for unexpected 1 nop before local >>>> - entry. */ >>>> +/* Verify there is no error with 1 nop before local entry. */ >>>> >>>> extern int a; >>>> >>>> int test (int b) { >>>> return a + b; >>>> } >>>> -/* { dg-error "unsupported number of nops before function entry >>>> \\(1\\)" "" { target *-*-* } .-1 } */ >>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr99888-5.c >>>> b/gcc/testsuite/gcc.target/powerpc/pr99888-5.c >>>> index 39d3b4465f1..13f192ebd20 100644 >>>> --- a/gcc/testsuite/gcc.target/powerpc/pr99888-5.c >>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-5.c >>>> @@ -2,12 +2,10 @@ >>>> /* There is no global entry point prologue with pcrel. */ >>>> /* { dg-options "-mno-pcrel -fpatchable-function-entry=7,3" } */ >>>> >>>> -/* Verify one error emitted for unexpected 3 nops before local >>>> - entry. */ >>>> +/* Verify no error emitted for 3 nops before local entry. */ >>>> >>>> extern int a; >>>> >>>> int test (int b) { >>>> return a + b; >>>> } >>>> -/* { dg-error "unsupported number of nops before function entry >>>> \\(3\\)" "" { target *-*-* } .-1 } */ >>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr99888-6.c >>>> b/gcc/testsuite/gcc.target/powerpc/pr99888-6.c >>>> index c6c18dcc7ac..431c69cae9a 100644 >>>> --- a/gcc/testsuite/gcc.target/powerpc/pr99888-6.c >>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-6.c >>>> @@ -2,8 +2,7 @@ >>>> /* There is no global entry point prologue with pcrel. */ >>>> /* { dg-options "-mno-pcrel" } */ >>>> >>>> -/* Verify one error emitted for unexpected 4 nops before local >>>> - entry. */ >>>> +/* Verify no error emitted for 4 nops before local entry. */ >>>> >>>> extern int a; >>>> >>>> @@ -11,4 +10,3 @@ __attribute__ ((patchable_function_entry (20, >>>> 4))) >>>> int test (int b) { >>>> return a + b; >>>> } >>>> -/* { dg-error "unsupported number of nops before function entry >>>> \\(4\\)" "" { target *-*-* } .-1 } */ >>>> -- >>>> 2.39.1 >> >
