Hi!
Sorry this took so long to review. "I lost track of this patch", what
can I say :-/
On Fri, Aug 14, 2020 at 03:31:05PM +0800, HAO CHEN GUI wrote:
> This patch adds non-relative jump table support on Power Linux. It
> implements ASM_OUTPUT_ADDR_VEC_ELT and adds four new expansions for
> non-relative jump table in rs6000.md. It also defines a rs6000
> option(mrelative-jumptables). If it's set to false, the non-relative
> jump table is picked up even PIC flag is set.
> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
> index 34776c8421e..626f2201d96 100644
> --- a/gcc/config/rs6000/linux64.h
> +++ b/gcc/config/rs6000/linux64.h
> @@ -324,7 +324,10 @@ 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 rs6000_relative_jumptables
> +
> +#undef JUMP_TABLES_IN_RELRO
> +#define JUMP_TABLES_IN_RELRO !rs6000_relative_jumptables
The third choice is .rodata. .data.rel.ro can be considered to be a
sub-case of that, but not the other way around.
> +extern void rs6000_output_addr_vec_elt(FILE *, int);
(space before opening paren)
> +/* Implement TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC. Return true if
> rs6000_relative_jumptables is set. */
The maximum line length is 80 characters. Two spaces after a full stop.
> +/* Enable non-relative jump tables for linux ELF. */
> +#ifdef TARGET_ELF
> +#if !TARGET_AIX
> +#undef rs6000_relative_jumptables
> +#define rs6000_relative_jumptables 0
> +#endif
> +#endif
AIX never is ELF, but many other things *are* ELF. Do all of those
handle .data.rel.ro, including do they have enough OS support for it?
This also changes the default for ELF systems? Let's not do that (in
this patch, that is).
> +/* Specify the machine mode that this machine uses
> + for the index in the tablejump instruction. */
> +#define CASE_VECTOR_MODE \
> + (TARGET_32BIT || rs6000_relative_jumptables ? SImode : DImode)
#define CASE_VECTOR_MODE (rs6000_relative_jumptables ? SImode : Pmode)
> +/* This is how to output an element of a case-vector
> + that is non-relative. */
(no space at end of line; two spaces after full stop, also before */)
> +#define ASM_OUTPUT_ADDR_VEC_ELT(FILE, VALUE) \
> + rs6000_output_addr_vec_elt((FILE), (VALUE))
Space before first ( please (on the second line, on the first you
cannot -- macro definitions are the only exception).
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 0aa5265d199..2f28da0ea1f 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -12669,18 +12669,32 @@
> if (rs6000_speculate_indirect_jumps)
> {
> if (TARGET_32BIT)
> - emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> - else
> + {
> + if (rs6000_relative_jumptables)
> + emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> + else
> + emit_jump_insn (gen_nonrelative_tablejumpsi (operands[0],
> operands[1]));
Please fix the indent while you're at it (tabs shouldn't follow spaces).
Oh, and line too long.
> + }
> + else if (rs6000_relative_jumptables)
> emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> + else
> + emit_jump_insn (gen_nonrelative_tablejumpdi (operands[0],
> operands[1]));
Please put the "relative" case on the outside, and the "32 or 64" if on
the inside.
> +(define_expand "nonrelative_tablejumpsi"
Maybe call it "absolute_tablejumpsi"? Not sure :-)
> + "TARGET_32BIT && rs6000_speculate_indirect_jumps &&
> !rs6000_relative_jumptables"
Line too long (more follow, I won't point all out, please check :-) )
> + {
> + operands[2] = gen_reg_rtx (SImode);
> + })
No spaces before { and } here, those are flush left like in outer braces
in a C function are.
> +(define_expand "nonrelative_tablejumpsi_nospec"
> + [(set (match_dup 3)
> + (match_operand:SI 0 "gpc_reg_operand" "r"))
> + (parallel [(set (pc)
> + (match_dup 3))
> + (use (label_ref (match_operand 1)))
> + (clobber (match_operand 2))])]
Please use a leading tab instead of every 8 leading spaces.
I'll try to be quicker at reviewing iterations of this -- there is quite
some way to go, without me slowing things down!
Segher
