From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33975 invoked by alias); 1 Dec 2016 14:40:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 33948 invoked by uid 89); 1 Dec 2016 14:40:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=BAYES_00,KAM_STOCKGEN,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=vary, as_a, relate, generator_file X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Dec 2016 14:40:30 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F16F613A5D for ; Thu, 1 Dec 2016 14:40:28 +0000 (UTC) Received: from localhost.localdomain (vpn1-7-50.ams2.redhat.com [10.36.7.50]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uB1EeQGU032073; Thu, 1 Dec 2016 09:40:27 -0500 Subject: Re: [PATCH 8/9] Introduce class function_reader (v4) To: David Malcolm , gcc-patches@gcc.gnu.org References: <1478898935-46932-1-git-send-email-dmalcolm@redhat.com> <1478898935-46932-9-git-send-email-dmalcolm@redhat.com> From: Bernd Schmidt Message-ID: <115e0b30-bb0a-7a37-d428-f706bbcf06dd@redhat.com> Date: Thu, 01 Dec 2016 14:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1478898935-46932-9-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00082.txt.bz2 On 11/11/2016 10:15 PM, David Malcolm wrote: > #include "gt-aarch64.h" > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 6c608e0..0dda786 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c I think we should separate out the target specific tests so as to give port maintainers a chance to comment on them separately. > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c > index 50cd388..179a91f 100644 > --- a/gcc/emit-rtl.c > +++ b/gcc/emit-rtl.c > @@ -1371,6 +1371,19 @@ maybe_set_first_label_num (rtx_code_label *x) > if (CODE_LABEL_NUMBER (x) < first_label_num) > first_label_num = CODE_LABEL_NUMBER (x); > } > + > +/* For use by the RTL function loader, when mingling with normal > + functions. Not sure what this means. > if (str == 0) > - fputs (" \"\"", m_outfile); > + fputs (" (nil)", m_outfile); > else > fprintf (m_outfile, " (\"%s\")", str); > m_sawclose = 1; What does this affect? > /* Global singleton; constrast with md_reader_ptr above. */ > diff --git a/gcc/read-rtl-function.c b/gcc/read-rtl-function.c > new file mode 100644 > index 0000000..ff6c808 > --- /dev/null > +++ b/gcc/read-rtl-function.c > @@ -0,0 +1,2124 @@ > +/* read-rtl-function.c - Reader for RTL function dumps > + Copyright (C) 2016 Free Software Foundation, Inc. > + > +This file is part of GCC. > + > +GCC is free software; you can redistribute it and/or modify it under > +the terms of the GNU General Public License as published by the Free > +#include Please double-check all these includes whether they are necessary. > + > +/* Fix up a NOTE_INSN_BASIC_BLOCK based on an integer block ID. */ > + > +void > +fixup_note_insn_basic_block::apply (function_reader */*reader*/) const Lose the /*reader*/, probably. > + > +/* Implementation of rtx_reader::handle_unknown_directive. > + > + Require a top-level "function" elements, as emitted by > + print_rtx_function, and parse it. */ "element"? > +void > +function_reader::create_function () > +{ > + /* Currently we assume cfgrtl mode, rather than cfglayout mode. */ > + if (0) > + cfg_layout_rtl_register_cfg_hooks (); > + else > + rtl_register_cfg_hooks (); Do we expect to change this? I'd just get rid of the if (0), at least for now. > + /* cgraph_node::add_new_function does additional processing > + based on symtab->state. We need to avoid it attempting to gimplify > + things. Temporarily putting it in the PARSING state appears to > + achieve this. */ > + enum symtab_state old_state = symtab->state; > + symtab->state = PARSING; > + cgraph_node::add_new_function (fndecl, true /*lowered*/); > + /* Reset the state. */ > + symtab->state = old_state; > + } Does it do anything beside call finalize_function in that state? If that's all you need, just call it directly. > + > + /* Parse DECL_RTL. */ > + { > + require_char_ws ('('); > + require_word_ws ("DECL_RTL"); > + DECL_WRTL_CHECK (t_param)->decl_with_rtl.rtl = parse_rtx (); > + require_char_ws (')'); > + } Spurious { } blocks. > + if (0) > + fprintf (stderr, "parse_edge: %i flags 0x%x \n", > + other_bb_idx, flags); Remove this. > + /* For now, only process the (edge-from) to this BB, and (edge-to) > + that go to the exit block; we don't yet verify that the edge-from > + and edge-to directives are consistent. */ That's probably worth a FIXME. > + if (rtx_code_label *label = dyn_cast (insn)) > + maybe_set_max_label_num (label); I keep forgetting why dyn_cast instead of as_a? > + case 'e': > + { > + if (idx == 7 && CALL_P (return_rtx)) > + { > + m_in_call_function_usage = true; > + return rtx_reader::read_rtx_operand (return_rtx, idx); > + m_in_call_function_usage = false; > + } > + else > + return rtx_reader::read_rtx_operand (return_rtx, idx); > + } > + break; Unnecessary { } blocks in several places. > + > + case 'w': > + { > + if (!is_compact ()) > + { > + /* Strip away the redundant hex dump of the value. */ > + require_char_ws ('['); > + read_name (&name); > + require_char_ws (']'); > + } > + } > + break; Here too. > + > +/* Special-cased handling of codes 'i' and 'n' for reading function > + dumps. */ > + > +void > +function_reader::read_rtx_operand_i_or_n (rtx return_rtx, int idx, > + char format_char) Document arguments (everywhere). I think return_rtx (throughout these functions) is a poor name that can cause confusion because it seems to imply a (return). > + > + /* Possibly wrote: > + print_node_brief (outfile, "", SYMBOL_REF_DECL (in_rtx), > + dump_flags); */ ??? > + /* Skip the content for now. */ Does this relate to the above? Please clarify the comments. > + case CODE_LABEL: > + { > + /* Assume that LABEL_NUSES was not dumped. */ > + /* TODO: parse LABEL_KIND. */ Unnecessary { }. > + if (0 == strcmp (desc, "")) > + { > + return DECL_RESULT (fndecl); > + } > + > + /* Search within function parms. */ > + for (tree arg = DECL_ARGUMENTS (fndecl); arg; arg = TREE_CHAIN (arg)) > + { > + if (strcmp (desc, IDENTIFIER_POINTER (DECL_NAME (arg))) == 0) > + return arg; > + } No { } around single statements, both cases. > + /* Not found? Create it. > + This allows mimicing of real data but avoids having to specify "mimicking". > +rtx > +function_reader::consolidate_singletons (rtx x) > +{ > + if (!x) > + return x; > + > + switch (GET_CODE (x)) Formatting. > + { > + /* FIXME: do we need to check for VOIDmode for these? */ Don't see why we would. > + case CONST_INT: > + return gen_rtx_CONST_INT (GET_MODE (x), INTVAL (x)); Ugh, really? Can't this be done earlier? > + > + add_fixup_source_location (loc, insn, operand_idx, > + filename, atoi(name.string)); Space before open paren. > + /* Verify lookup of hard registers. */ > +#ifdef GCC_AARCH64_H > + ASSERT_EQ (0, lookup_reg_by_dump_name ("x0")); > + ASSERT_EQ (1, lookup_reg_by_dump_name ("x1")); > +#endif > +#ifdef I386_OPTS_H > + ASSERT_EQ (0, lookup_reg_by_dump_name ("ax")); > + ASSERT_EQ (1, lookup_reg_by_dump_name ("dx")); > +#endif Same as before, please don't do this here. > +/* Verify that the src and dest indices and flags of edge E are as > + expected, using LOC as the effective location when reporting > + failures. */ Again, please document all args (everywhere). > + > +static void > +test_loading_dump_fragment_1 () > +{ > + // TODO: filter on target? > + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("asr_div1.rtl")); This is in a generic file, isn't it? Yeah, I don't think we want to load any RTL from here. Split out such selftests into a separate patch so we can figure out what to do with them. > + > + rtx_insn *jump_insn = get_insn_by_uid (1); > + ASSERT_EQ (JUMP_INSN, GET_CODE (jump_insn)); > + ASSERT_EQ (ret_rtx, JUMP_LABEL (jump_insn)); > + // FIXME: ^^^ use ASSERT_RTX_PTR_EQ here ^^^ Why is this a fixme and not just done that way (several instances)? > + > +/* An optional policy class for class function_reader. */ > + > +struct function_reader_policy > +{ > + function_reader_policy () > + { > + } > +}; > + > +extern bool read_rtl_function_body (int argc, const char **argv, > + bool (*parse_opt) (const char *), > + function_reader_policy *policy); The policy seems completely unused, please remove. Also, can't this single declaration go into an existing header? > + /* Handle insn codes in compact dumps. In such dumps, the code of insns > + is prefixed with "c", giving "cinsn", "cnote" etc, and CODE_LABEL is > + special-cased as "clabel". */ > + if (name[0] == 'c') > + { > + for (i = 0; i < NUM_RTX_CODE; i++) > + if (strcmp (GET_RTX_NAME (i), name + 1) == 0) > + return i; I'd rather check specifically for the ones we expect, to avoid surprises. > +#ifdef GENERATOR_FILE There's a lot of this. Is there a clean split where stuff could be moved into a separate file instead? > @@ -1103,13 +1233,39 @@ rtx_reader::read_rtx_code (const char *code_name) > rtx value; /* Value of this node. */ > }; > > + one_time_initialization (); Isn't there a better place to call this? > + > + /* Use the format_ptr to parse the various operands of this rtx. > + read_rtx_operand is a vfunc, allowing the parser to vary between > + parsing .md files and parsing .rtl dumps; the function_reader subclass > + overrides the defaults when loading RTL dumps, to handle the > + various extra attributes emitted by print_rtx. */ Not sure that documentation is really necessary at this point. If someone is looking for the definition of read_rtx_operand they'll figure out it's a virtual function anyway. > for (int idx = 0; format_ptr[idx] != 0; idx++) > - read_rtx_operand (return_rtx, idx); > + return_rtx = read_rtx_operand (return_rtx, idx); > + > + /* Call a vfunc to handle the various additional information that > + print-rtl.c can write after the regular fields; does nothing when > + parsing .md files. */ > + handle_any_trailing_information (return_rtx); Same here really. > + > + /* "stringbuf" was allocated within string_obstack and thus has > + the its lifetime restricted to that of the rtx_reader. This is > + OK for the generator programs, but for non-generator programs, > + XSTR and XTMPL fields are meant to be allocated in the GC-managed > + heap. Hence we need to allocate a copy in the GC-managed heap > + for the non-generator case. */ > + const char *string_ptr = stringbuf; > +#ifndef GENERATOR_FILE > + string_ptr = ggc_strdup (stringbuf); > +#endif /* #ifndef GENERATOR_FILE */ Encapsulate in a virtual finalize_string perhaps? Bernd