From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34686 invoked by alias); 8 Oct 2015 20:01: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 34675 invoked by uid 89); 8 Oct 2015 20:01:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vk0-f50.google.com Received: from mail-vk0-f50.google.com (HELO mail-vk0-f50.google.com) (209.85.213.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 08 Oct 2015 20:01:30 +0000 Received: by vkao3 with SMTP id o3so39034292vka.2 for ; Thu, 08 Oct 2015 13:01:28 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.31.148.206 with SMTP id w197mr6435381vkd.100.1444334488265; Thu, 08 Oct 2015 13:01:28 -0700 (PDT) Received: by 10.31.10.14 with HTTP; Thu, 8 Oct 2015 13:01:28 -0700 (PDT) In-Reply-To: <5616BD37.2000307@redhat.com> References: <56000538.5000800@t-online.de> <5601AEDB.4000100@redhat.com> <5601C375.5050706@redhat.com> <5616BD37.2000307@redhat.com> Date: Thu, 08 Oct 2015 20:01:00 -0000 Message-ID: Subject: Re: [PATCH] New attribute to create target clones From: Evgeny Stupachenko To: Jeff Law Cc: Bernd Schmidt , Bernd Schmidt , GCC Patches , Jan Hubicka Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00900.txt.bz2 On Thu, Oct 8, 2015 at 10:00 PM, Jeff Law wrote: > On 09/24/2015 04:28 PM, Evgeny Stupachenko wrote: >> >> I've fixed ICE and review issues. >> x86 make check and bootstrap passed. >> >> Thanks, >> Evgeny >> >> ChangeLog >> >> 2015-09-25 Evgeny Stupachenko >> >> gcc/ >> * Makefile.in (OBJS): Add multiple_target.o. >> * multiple_target.c (make_attribute): New. >> (create_dispatcher_calls): Ditto. >> (expand_target_clones): Ditto. >> (ipa_target_clone): Ditto. >> * passes.def (pass_target_clone): New ipa pass. >> * tree-pass.h (make_pass_target_clone): Ditto. >> >> gcc/c-family >> * c-common.c (handle_target_clones_attribute): New. >> * (c_common_attribute_table): Add handle_target_clones_attribute. >> * (handle_always_inline_attribute): Add check on target_clones >> attribute. >> * (handle_target_attribute): Ditto. >> >> gcc/testsuite >> * gcc.dg/mvc1.c: New test for multiple targets cloning. >> * gcc.dg/mvc2.c: Ditto. >> * gcc.dg/mvc3.c: Ditto. >> * gcc.dg/mvc4.c: Ditto. >> * gcc.dg/mvc5.c: Ditto. >> * gcc.dg/mvc6.c: Ditto. >> * gcc.dg/mvc7.c: Ditto. >> * g++.dg/ext/mvc1.C: Ditto. >> * g++.dg/ext/mvc2.C: Ditto. >> * g++.dg/ext/mvc3.C: Ditto. >> >> gcc/doc >> * doc/extend.texi (target_clones): New attribute description. >> >> > >> >> target_clones.patch > > Sorry this has taken so long to come back to... As I mentioned a couple > months ago, I'd hoped Jan would chime in on the IPA/symtab requirements. > But that didn't happen. > > > SO I went back and reviewed the discussion between Jan, Ilya & myself WRT > some of the rules around aliases, clones, etc. I think the key question for > this patch is whether or not the clones have the same assembler name or not. > From looking at expand_target_clones, I'm confident the answer is the clones > have different assembler names. In fact, the assembler names are munged > with the options used for that specific clone of the original function. > > >> >> +/* Makes a function attribute of the form NAME(ARG_NAME) and chains >> + it to CHAIN. */ >> + >> +static tree >> +make_attribute (const char *name, const char *arg_name, tree chain) >> +{ >> + tree attr_name; >> + tree attr_arg_name; >> + tree attr_args; >> + tree attr; >> + >> + attr_name = get_identifier (name); >> + attr_arg_name = build_string (strlen (arg_name), arg_name); >> + attr_args = tree_cons (NULL_TREE, attr_arg_name, NULL_TREE); >> + attr = tree_cons (attr_name, attr_args, chain); >> + return attr; >> +} > > This seems generic enough that I'd prefer it in attribs.c. I was rather > surprised when I looked and didn't find an existing routine to do this. > > >> + >> +/* If the call in NODE has multiple target attribute with multiple >> fields, >> + replace it with dispatcher call and create dispatcher (once). */ >> + >> +static void >> +create_dispatcher_calls (struct cgraph_node *node) >> +{ >> + cgraph_edge *e; >> + cgraph_edge *e_next; >> + for (e = node->callers; e ;e = (e == NULL) ? e_next : e->next_caller) > > That's a rather strange way to write the loop increment. If I follow the > loop logic correctly, it seems that we always end up using e->next_caller, > it's just obscured. > > For the test if we're calling a versioned function, we just "continue". e > will be non-null and thus we use e->next_caller to set e for the next > iteration. > > If the test for calling a versioned function falls, we set e_next to > e->next_caller, then later set e to NULL. That results in using e_next to > set e for the next iteration. But e_next was initialized to e->next_caller. > > So why not just write the loop increment as e = e->next-caller? Because of this: e->redirect_callee (inode); It modifies next_caller field. The other way is to remember all what we want to redirect and create one more loop through the modifications to apply them. > > > >> + { >> + tree resolver_decl; >> + tree idecl; >> + tree decl; >> + gimple *call = e->call_stmt; >> + struct cgraph_node *inode; >> + >> + /* Checking if call of function is call of versioned function. >> + Versioned function are not inlined, so there is no need to >> + check for inline. */ > > This comment doesn't parse well. Perhaps: > > /* Check if this is a call to a versioned function. Verisoned > fucntions are not inlined, so there is no need to check for that. */ > > >> + >> +/* If the function in NODE has multiple target attribute with multiple >> fields, >> + create the appropriate clone for each field. */ >> + >> +static bool >> +expand_target_clones (struct cgraph_node *node) > > So this is probably getting a little large. Can we look to refactor it a > little? It's not a huge deal and there's certainly code in GCC that is far > worse, but it just feels like there's enough going on in this code that > there ought to be 2-3 helpers for the larger chunks of work going on inside > this code. > > The first is the attribute parsing. The second is creation of the nodes, > and the final would be munging the name and attributes on the clones. > > I'm slightly concerned with using the pretty printer to build up the new > name. Is there precedent for this anywhere else in GCC? I don't remember where it exactly came from. However it's not a big deal to simplify this to std functions. > > When creating the munged name, don't you also have to make sure that other > symbols that aren't supported for names don't sneak through? I see that you > replace = and -, but you'd need to replace any symbol that could possibly be > used in an option, but which isn't allowed in a function name at the > assembler level. I'd be worried about anything that might possibly be seen > as an operator by the assembler, '.', and possibly others. This restriction comes from "targetm.target_option.valid_attribute_p" and it is the same for current implementation of function multiversioning. It exits with error: "attribute(target("...")) is unknown". It looks reasonable to put the check before symtab changes. > > Also, please double check indentation of that function. In particular the > code after the if-then-else (node->definition) looks like it's indented too > far. > > I think this is pretty close, but does need another iteration. Now that > we've moved past the interactions with IPA question and have fairly > carefully looked at the rest of the patch, I don't think subsequent reviews > will take much time at all. > > jeff Thanks for the review, Evgeny