From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96440 invoked by alias); 10 Jul 2015 19:54:41 -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 96431 invoked by uid 89); 10 Jul 2015 19:54:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-oi0-f51.google.com Received: from mail-oi0-f51.google.com (HELO mail-oi0-f51.google.com) (209.85.218.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 10 Jul 2015 19:54:39 +0000 Received: by oiab3 with SMTP id b3so100268075oia.1 for ; Fri, 10 Jul 2015 12:54:37 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.202.55.7 with SMTP id e7mr19626219oia.56.1436558077455; Fri, 10 Jul 2015 12:54:37 -0700 (PDT) Received: by 10.60.231.195 with HTTP; Fri, 10 Jul 2015 12:54:37 -0700 (PDT) In-Reply-To: References: <20150709105439.GA19247@gmail.com> Date: Fri, 10 Jul 2015 19:54:00 -0000 Message-ID: Subject: Re: [PATCH] PR target/66819: Allow indirect sibcall with register arguments From: Uros Bizjak To: "H.J. Lu" Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-07/txt/msg00922.txt.bz2 On Fri, Jul 10, 2015 at 7:58 PM, H.J. Lu wrote: > On Fri, Jul 10, 2015 at 10:21 AM, Uros Bizjak wrote: >> On Fri, Jul 10, 2015 at 7:10 PM, H.J. Lu wrote: >>> On Fri, Jul 10, 2015 at 9:30 AM, Uros Bizjak wrote: >>>> On Thu, Jul 9, 2015 at 12:54 PM, H.J. Lu wrote: >>>>> Indirect sibcall with register arguments is OK when there is register >>>>> available for argument passing. >>>>> >>>>> OK for trunk if there is no regression? >>>>> >>>>> >>>>> H.J. >>>>> --- >>>>> gcc/ >>>>> >>>>> PR target/66819 >>>>> * config/i386/i386.c (ix86_function_ok_for_sibcall): Allow >>>>> indirect sibcall with register arguments if register available >>>>> for argument passing. >>>>> (init_cumulative_args): Set cfun->machine->arg_reg_available_p >>>>> to cum->nregs != 0. >> >> Please update the above entry for nregs > 0. >> >>>>> (function_arg_advance_32): Set cfun->machine->arg_reg_available_p >>>>> to 0 when setting cum->nregs = 0. >>>> >>>> Do we also need similar functionality for 64bit ABIs? What happens if >>>> we are out of argument regs there? >>> >>> 64-bit is OK since we have rax, r10 and r11 as scratch registers which >>> aren't used to pass arguments. >> >> Maybe this fact should be added as a comment in some appropriate place. >> >>>>> * config/i386/i386.h (machine_function): Add arg_reg_available_p. >>>>> >>>>> gcc/testsuite/ >>>>> >>>>> PR target/66819 >>>>> * gcc.target/i386/pr66819-1.c: New test. >>>>> * gcc.target/i386/pr66819-2.c: Likewise. >>>>> * gcc.target/i386/pr66819-3.c: Likewise. >>>>> * gcc.target/i386/pr66819-4.c: Likewise. >>>>> * gcc.target/i386/pr66819-5.c: Likewise. >>>>> --- >>>>> gcc/config/i386/i386.c | 15 +++++++++------ >>>>> gcc/config/i386/i386.h | 3 +++ >>>>> gcc/testsuite/gcc.target/i386/pr66819-1.c | 8 ++++++++ >>>>> gcc/testsuite/gcc.target/i386/pr66819-2.c | 8 ++++++++ >>>>> gcc/testsuite/gcc.target/i386/pr66819-3.c | 10 ++++++++++ >>>>> gcc/testsuite/gcc.target/i386/pr66819-4.c | 12 ++++++++++++ >>>>> gcc/testsuite/gcc.target/i386/pr66819-5.c | 10 ++++++++++ >>>>> 7 files changed, 60 insertions(+), 6 deletions(-) >>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-1.c >>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-2.c >>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-3.c >>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-4.c >>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr66819-5.c >>>>> >>>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>>>> index 54ee6f3..85e59a8 100644 >>>>> --- a/gcc/config/i386/i386.c >>>>> +++ b/gcc/config/i386/i386.c >>>>> @@ -5628,12 +5628,12 @@ ix86_function_ok_for_sibcall (tree decl, tree exp) >>>>> if (!decl >>>>> || (TARGET_DLLIMPORT_DECL_ATTRIBUTES && DECL_DLLIMPORT_P (decl))) >>>>> { >>>>> - if (ix86_function_regparm (type, NULL) >= 3) >>>>> - { >>>>> - /* ??? Need to count the actual number of registers to be used, >>>>> - not the possible number of registers. Fix later. */ >>>>> - return false; >>>>> - } >>>>> + /* FIXME: The symbol indirect call doesn't need a >>>>> + call-clobbered register. But we don't know if >>>>> + this is a symbol indirect call or not here. */ >>>>> + if (ix86_function_regparm (type, NULL) >= 3 >>>>> + && !cfun->machine->arg_reg_available_p) >>>> >>>> Isn't enough to look at arg_reg_available here? >>> >>> We need to check ix86_function_regparm since nregs is 0 if >>> -mregparm=N isn't used and pr65753.c will fail. >> >> OK. Please add this comment, is not that obvious. >> >>> >>>>> + return false; >>>>> } >>>>> } >>>>> >>>>> @@ -6567,6 +6567,7 @@ init_cumulative_args (CUMULATIVE_ARGS *cum, /* Argument info to initialize */ >>>>> ? X86_64_REGPARM_MAX >>>>> : X86_64_MS_REGPARM_MAX); >>>>> } >>>>> + cfun->machine->arg_reg_available_p = cum->nregs != 0; >>>> >>>> false instead of 0. This is a boolean. >>> >>> Updated. >>> >>>>> if (TARGET_SSE) >>>>> { >>>>> cum->sse_nregs = SSE_REGPARM_MAX; >>>>> @@ -6636,6 +6637,7 @@ init_cumulative_args (CUMULATIVE_ARGS *cum, /* Argument info to initialize */ >>>>> else >>>>> cum->nregs = ix86_function_regparm (fntype, fndecl); >>>>> } >>>>> + cfun->machine->arg_reg_available_p = cum->nregs != 0; >>>> >>>> IMO, cum->nregs > 0 would be more descriptive. >>> >>> Updated. >>> >>>>> /* Set up the number of SSE registers used for passing SFmode >>>>> and DFmode arguments. Warn for mismatching ABI. */ >>>>> @@ -7584,6 +7586,7 @@ pass_in_reg: >>>>> { >>>>> cum->nregs = 0; >>>>> cum->regno = 0; >>>>> + cfun->machine->arg_reg_available_p = 0; >>>>> } >>>>> break; >>>>> >>>>> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h >>>>> index 74334ff..0b6e304 100644 >>>>> --- a/gcc/config/i386/i386.h >>>>> +++ b/gcc/config/i386/i386.h >>>>> @@ -2479,6 +2479,9 @@ struct GTY(()) machine_function { >>>>> /* If true, it is safe to not save/restore DRAP register. */ >>>>> BOOL_BITFIELD no_drap_save_restore : 1; >>>>> >>>>> + /* If true, there is register available for argument passing. */ >>>>> + BOOL_BITFIELD arg_reg_available_p : 1; >>>> >>>> This is not a predicate, but a boolean flag. Please remove _p from the name. >>> >>> Updated. >>> >>> Here is the updated patch. OK for trunk? >> >> OK with a small comment additions. >> >> + /* If true, there is register available for argument passing. */ >> + BOOL_BITFIELD arg_reg_available : 1; >> + >> >> Please mention here that this is for 32bit targets only. >> > > Updated. Is this one OK? LGTM. OK for mainline. Thanks, Uros.