From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id D5BF83858D35 for ; Thu, 30 Jul 2020 23:42:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D5BF83858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 06UNgjFZ028976; Thu, 30 Jul 2020 18:42:45 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 06UNgiBQ028975; Thu, 30 Jul 2020 18:42:44 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 30 Jul 2020 18:42:44 -0500 From: Segher Boessenkool To: HAO CHEN GUI Cc: gcc-patches@gcc.gnu.org, David Edelsohn , Bill Schmidt Subject: Re: [PATCH, rs6000] Add non-relative jump table support for 64bit rs6000 Message-ID: <20200730234244.GB6753@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 23:42:47 -0000 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