Re: [PATCH, rs6000] Enable absolute jump table by default

[David Edelsohn <dje.gcc@gmail.com>]

On Wed, Jan 12, 2022 at 8:40 PM HAO CHEN GUI <guihaoc@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 <guihaoc@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 <guihaoc@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