Hi!
As I explained in the other mail, please split this into two: one the
core thing, adding the flag and all code generation (maybe using some
new macros or such for section selection, for example); and the second
patch hooking it up and enabling this by default for powerpc64* (or only
for powerpc64le?)
The first patch should work on all OSes, all endians, all word sizes.
On Tue, Jul 28, 2020 at 01:25:36PM +0800, HAO CHEN GUI via Gcc-patches wrote:
> We want to put non-relative jump
> table in data.rel.ro section for rs6000. So I add a new target hook -
> TARGET_ASM_SELECT_JUMPTABLE_SECTION to choose which section jump table
> should be put in.
That sounds like a good idea, but please put it in a separate patch as
well? Before everything else.
> It puts the jump table in data.rel.ro for 64bit
> rs6000. For other platforms, it calls
> targetm.asm_out.function_rodata_section, just as before.
Maybe that hook should get another parameter, instead?
> We want to have
> an option to use non-relative jump table even when PIC flag is set. So I
> add a new option - "mforce-nonrelative-jumptables" for rs6000.
"force" isn't good in a name, and no negatives please (what would
-mno-force-norelative-jumptables mean?) So just -mrelative-jumptables
then?
> static bool
> rs6000_gen_pic_addr_diff_vec (void)
> {
> return TARGET_32BIT || !rs6000_force_nonrelative_jumptables;
> }
There is no reason why it cannot work on 32-bit? It even is more
obviously a win there (for 64-bit the jump table elements are just 32
bits for relative, but 64 otherwise; for 32-bit, you get 32 bits either
way, so that isn't an advantage for relative there).
> * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Redefine.
> * config/rs6000/rs6000.c
> (TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC,TARGET_ASM_SELECT_JUMPTABLE_SECTION):
> Implement two hooks.
(Space after comma. Line too long.)
The target macros are not very directly related, so put them in separate
entries please?
> * config/rs6000/rs6000.h
> (CASE_VECTOR_PC_RELATIVE,CASE_VECTOR_MODE) Redefine.
Space after comma, colon after closing paren here.
"Redefine"? The patch doesn't do that (which is good, we don't want
that). Ah you mean the patch changes the value it is defined as; your
changelog sounds like it now #undefs it first, which would be nasty :-)
> * config/rs6000/rs6000.md (nonrelative_tablejumpdi,
> nonrelative_tablejumpdi_nospec) Add two expansions.
> * config/rs6000/rs6000.opt (mforce-nonrelative-jumptables) Add
> a new options for rs6000.
Colon, line too long. Changelog lines are 80 chars max, including the
initial tab (which counts as 8 spaces).
> * doc/tm.texi.in (TARGET_ASM_SELECT_JUMPTABLE_SECTION): Likewise.
Likewise? It doesn't do the same thing as the previous line.
> * doc/tm.texi: Regenerate.
> * final.c (targetm.asm_out.select_jumptable_section): Replace
> targetm.asm_out.function_rodata_section with
> targetm.asm_out.select_jumptable_section in jumptable section selection.
> * output.h (default_select_jumptable_section): Declare.
> * target.def (default_select_jumptable_section): Likewise
> * varasm.c (default_select_jumptable_section): Implementation.
> --- a/gcc/config/rs6000/linux64.h
> +++ b/gcc/config/rs6000/linux64.h
> @@ -324,7 +324,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 (TARGET_64BIT && flag_pic &&
> !rs6000_force_nonrelative_jumptables)
I'm not sure where flag_pic matters here?
> +/* Implement TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC. Return false if
> rs6000_force_nonrelative_jumptables is set and target is 64bit. */
Line too long. Lines of code are at most 80 chars.
> +/* Implement TARGET_ASM_FUNCTION_RODATA_SECTION. Put non-relative jump table
> in data.rel.ro section */
> +
> +section *
> +rs6000_select_jumptable_section (tree decl)
> +{
> + if (TARGET_32BIT)
> + return default_function_rodata_section (decl);
This shouldn't exclude 32 bit?
> + if (decl != NULL_TREE && DECL_SECTION_NAME (decl))
Just "if (decl && DECL_SECTION_NAME (decl))" if you ask me... but some
people disagree, I guess.
> + {
> + const char *name = DECL_SECTION_NAME (decl);
> +
> + if (DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP)
> + {
> + const char *dot;
> + size_t len;
> + char* rname;
> +
> + dot = strchr (name + 1, '.');
> + if (!dot)
> + dot = name;
> + len = strlen (dot) + 13;
> + rname = (char *) alloca (len);
> +
> + strcpy (rname, ".data.rel.ro");
> + strcat (rname, dot);
> + return get_section (rname, SECTION_WRITE | SECTION_RELRO |
> SECTION_LINKONCE, decl);
> + }
> + /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */
> + else if (DECL_COMDAT_GROUP (decl)
> + && strncmp (name, ".gnu.linkonce.t.", 16) == 0)
> + {
> + size_t len = strlen (name) + 1;
> + char *rname = (char *) alloca (len);
> +
> + memcpy (rname, name, len);
> + rname[14] = 'r';
> + return get_section (rname, SECTION_LINKONCE, decl);
> + }
> + /* For .text.foo we want to use .data.rel.ro.foo. */
> + else if (flag_function_sections && flag_data_sections
> + && strncmp (name, ".text.", 6) == 0)
> + {
> + size_t len = strlen (name) + 1;
> + char *rname = (char *) alloca (len + 7);
> +
> + memcpy (rname, ".data.rel.ro", 12);
> + memcpy (rname + 12, name + 5, len - 5);
> + return get_section (rname, SECTION_WRITE | SECTION_RELRO, decl);
> + }
> + }
> + return get_section (".data.rel.ro", SECTION_WRITE | SECTION_RELRO, decl);
> +}
So this is hugely complicated. I think most of it is copy/paste from
somewhere else? Can this be refactored, instead?
> > It puts the jump table in data.rel.ro for 64bit
> > rs6000. For other platforms, it calls
> > targetm.asm_out.function_rodata_section, just as before.
>
> Maybe that hook should get another parameter, instead?
^^^ Perhaps via that?
> -/* Specify the machine mode that this machine uses
> - for the index in the tablejump instruction. */
> -#define CASE_VECTOR_MODE SImode
> +#define CASE_VECTOR_MODE (TARGET_32BIT || (flag_pic &&
> !rs6000_force_nonrelative_jumptables) ? SImode : DImode)
That will be just
#define CASE_VECTOR_MODE (rs6000_relative_jumptables ? SImode : Pmode)
or similar?
> /* Define as C expression which evaluates to nonzero if the tablejump
> instruction expects the table to contain offsets from the address of the
> table.
> Do not define this if the table should contain absolute addresses. */
> -#define CASE_VECTOR_PC_RELATIVE 1
> +#define CASE_VECTOR_PC_RELATIVE TARGET_32BIT
#define CASE_VECTOR_PC_RELATIVE rs6000_relative_jumptables
> +/* This is how to output an element of a case-vector that is non-relative. */
> +#define ASM_OUTPUT_ADDR_VEC_ELT(FILE, VALUE) \
> + do { char buf[100]; \
> + fputs ("\t.quad ", FILE); \
> + ASM_GENERATE_INTERNAL_LABEL (buf, "L", VALUE); \
> + assemble_name (FILE, buf); \
> + putc ('\n', FILE); \
> + } while (0)
That's not how we indent stuff. We shouldn't have big macros like this
anyway (make a function for it).
Use fprintf please, not the various "put" things.
Why would 100 be enough?
And see David's comment about .quad not existing on all assemblers we
support.
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -12670,8 +12670,10 @@
> {
> if (TARGET_32BIT)
> emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> - else
> + else if (flag_pic && !rs6000_force_nonrelative_jumptables)
> emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> + else
No space at end of line please.
> + emit_jump_insn (gen_nonrelative_tablejumpdi (operands[0],
> operands[1]));
I don't see why flag_pic matters here, and it should just work for 32-bit
as well.
> +(define_expand "nonrelative_tablejumpdi"
> + [(set (match_dup 2)
> + (match_operand:DI 0 "gpc_reg_operand" "r"))
> + (parallel [(set (pc)
> + (match_dup 2))
(Use a tab instead of every 8 leading spaces).
> +mforce-nonrelative-jumptables
> +Target Undocumented Var(rs6000_force_nonrelative_jumptables) Init(1) Save
"Undocumented", good, let's not make this an official option (for always
and ever) unless there turns out to be a reason for that.
This is a pretty big thing to do as your first patch :-) It will take a
few iterations to get right, don't let it discourage you!
Oh, btw, you don't have an entry in MAINTAINERS yet?
Segher
