On Wed, Jan 12, 2022 at 8:40 PM HAO CHEN GUI <guih...@linux.ibm.com> wrote:
>
> Hi David,
>
> On 12/1/2022 下午 10:44, David Edelsohn wrote:
> > On Wed, Jan 12, 2022 at 7:22 AM HAO CHEN GUI <guih...@linux.ibm.com> wrote:
> >>
> >> Hi,
> >> This patch enables absolute jump table by default on rs6000. The
> >> relative jump tables are used when
> >> it's explicit set by "rs6000_relative_jumptables",
> >> or jump tables are placed in text section but global relocation is
> >> required.
> >>
> >> Bootstrapped and tested on powerpc64-linux BE and LE with no
> >> regressions. Is this okay for trunk?
> >> Any recommendations? Thanks a lot.
> >>
> >> ChangeLog
> >> 2022-01-12 Haochen Gui <guih...@linux.ibm.com>
> >>
> >> gcc/
> >> * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
> >> * config/rs6000/rs6000.c (rs6000_gen_pic_addr_diff_vec): Return
> >> true when relative jump table is explicit required or jump tables
> >> have
> >> to be put in text section but global relocation is also required.
> >> * config/rs6000/rs6000.opt (rs6000_relative_jumptables): Disable.
> >>
> >> patch.diff
> >> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
> >> index d617f346f81..2e257c60f8c 100644
> >> --- a/gcc/config/rs6000/linux64.h
> >> +++ b/gcc/config/rs6000/linux64.h
> >> @@ -239,7 +239,7 @@ extern int dot_symbols;
> >>
> >> /* Indicate that jump tables go in the text section. */
> >> #undef JUMP_TABLES_IN_TEXT_SECTION
> >> -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT
> >> +#define JUMP_TABLES_IN_TEXT_SECTION 0
> >>
> >> /* The linux ppc64 ABI isn't explicit on whether aggregates smaller
> >> than a doubleword should be padded upward or downward. You could
> >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> >> index 319182e94d9..9fba893a27a 100644
> >> --- a/gcc/config/rs6000/rs6000.c
> >> +++ b/gcc/config/rs6000/rs6000.c
> >> @@ -28465,7 +28465,9 @@ rs6000_emit_xxspltidp_v2df (rtx dst, long value)
> >> static bool
> >> rs6000_gen_pic_addr_diff_vec (void)
> >> {
> >> - return rs6000_relative_jumptables;
> >> + return rs6000_relative_jumptables
> >> + || (JUMP_TABLES_IN_TEXT_SECTION
> >> + && targetm.asm_out.reloc_rw_mask () == 3);
> >> }
> >
> > This seems like contorted logic and overriding the
> > rs6000_relative_jumptables option change. The later part of the patch
> > overrides rs6000_relative_jumptables for all rs6000 configurations,
> > and then changes this one use of rs6000_relative_jumptables to add
> > more logic to revert to the old meaning for some targets.
> >
> > What about all of the other uses of rs6000_relative_jumptables in the
> > target? What about rs6000.md?
> >
> > I highly doubt that this patch is correct.
> >
> > Why not override rs6000_relative_jumptables for PPC64 Linux instead of
> > changing its value globally and then trying to fix it up?
> >
> > Thanks, David
> Thanks for your comments.
>
> In this patch, I tried to enable absolute jump table on all rs6000 targets.
> For PPC64 Linux, it supports RELRO section (e.g. .data.rel.ro.local) as
> "JUMP_TABLES_IN_TEXT_SECTION" is set to 0. So, absolute jump tables could be
> placed
> in RELRO section whatever global relocation is required or not. The absolute
> jump
> table can't be placed in text section when global relocation is required as
> text
> section is not writable. So for other rs6000 targets, absolute jump table
> can't be
> used if the target doesn't support RELRO and global relocation is required
> also.
>
> Looking forward to your advice. Thanks again.
Why not override rs6000_relative_jumptables in
rs6000_option_override_internal() for PPC64 Linux subtarget instead of
changing the default value and then trying to fix the behavior for
other configurations in rs6000_gen_pic_addr_diff_vec()? Or override
it in SUBSUBTARGET_OVERRIDE_OPTIONS in linux64.h, or whichever header
file(s) is appropriate for the subtarget variants.
Your initial patch also changed rs6000_gen_pic_addr_diff_vec but
didn't address the use of rs6000_relative_jumptables in the definition
of CASE_VECTOR_MODE in rs6000.h. That cannot have been correct. At
least without the change to the default value of
rs6000_relative_jumptables you don't need to add kludges to all of the
places where that variable is used for other subtarget variants of the
rs6000 target.
Thanks, David
