From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130799 invoked by alias); 27 Oct 2015 18:54:34 -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 130789 invoked by uid 89); 27 Oct 2015 18:54:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.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-ob0-f178.google.com Received: from mail-ob0-f178.google.com (HELO mail-ob0-f178.google.com) (209.85.214.178) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 27 Oct 2015 18:54:30 +0000 Received: by obctp1 with SMTP id tp1so151335004obc.2 for ; Tue, 27 Oct 2015 11:54:28 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.60.102.33 with SMTP id fl1mr28957684oeb.11.1445972068749; Tue, 27 Oct 2015 11:54:28 -0700 (PDT) Received: by 10.76.72.3 with HTTP; Tue, 27 Oct 2015 11:54:28 -0700 (PDT) In-Reply-To: <562FB812.7050601@redhat.com> References: <562F5E11.1090503@redhat.com> <562F739F.2090000@foss.arm.com> <562F818A.90003@foss.arm.com> <562F8B6F.7060605@foss.arm.com> <562F9B4C.8000607@foss.arm.com> <562FB812.7050601@redhat.com> Date: Tue, 27 Oct 2015 19:31:00 -0000 Message-ID: Subject: Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86 From: "H.J. Lu" To: Jeff Law Cc: Ramana Radhakrishnan , Jiong Wang , Bernd Schmidt , GCC Patches , Marcus Shawcroft Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg02947.txt.bz2 On Tue, Oct 27, 2015 at 10:44 AM, Jeff Law wrote: > On 10/27/2015 09:42 AM, Ramana Radhakrishnan wrote: >> >> >> >> On 27/10/15 14:50, H.J. Lu wrote: >>> >>> On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan >>> wrote: >>>> >>>> >>>>> >>>>> OK, then it's fairly x86-64 specific optimization, because we can't do >>>>> "call *mem" in >>>>> aarch64 and some other targets. >>>> >>>> >>>> It is a fairly x86_64 specific optimization and doesn't apply to >>>> AArch64. >>>> >>>> The question really is what impact does removing the generic code >>>> handling have on aarch64 - is it a no-op or not for the existing -fno-plt >>>> implementation in the AArch64 backend ? The only case that is of interest is >>>> the bit below in calls.c and it looks like that may well be redundant with >>>> the logic in the backend already, but I have not done the full analysis to >>>> convince myself that the code in the backend is sufficient. >>>> >>>> - && (!flag_plt >>>> - || lookup_attribute ("noplt", DECL_ATTRIBUTES >>>> (fndecl_or_type))) >>>> - && !targetm.binds_local_p (fndecl_or_type)) >>>> >>> >>> -fno-plt is a backend specific optimization and should be handled >>> in backend. >>> >> >> HJ, Thanks for committing the change even when we were discussing the >> change > > This is what I'm primarily concerned about. > > Bernd's message was pretty clear in my mind: > > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02861.html > > It was conditional approval based on no other target using -fno-plt and > agreement from the x86 maintainers. > > HJ replied that aarch64 uses -fno-plt: > > > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02865.html > > > And then apparently HJ committed the patch. > > > commit 47b727e5ec3f6f4f0a30ee899adce80185ad6999 > Author: hjl > Date: Tue Oct 27 14:29:31 2015 +0000 > > When reviewers conditionally approve a patch, the conditions need to be > satisfied before a patch can be committed. Ignoring the conditions seems > like a significant breech of trust to me. > > HJ, why did you commit the patch given it didn't meet the conditions Bernd > set forth for approval? > Sorry for the trouble my patch caused. The bug is in aarch64 backend. There are (define_expand "call" [(parallel [(call (match_operand 0 "memory_operand" "") (match_operand 1 "general_operand" "")) (use (match_operand 2 "" "")) (clobber (reg:DI LR_REGNUM))])] "" " { rtx callee, pat; /* In an untyped call, we can get NULL for operand 2. */ if (operands[2] == NULL) operands[2] = const0_rtx; /* Decide if we should generate indirect calls by loading the 64-bit address of the callee into a register before performing the branch-and-link. */ callee = XEXP (operands[0], 0); if (GET_CODE (callee) == SYMBOL_REF ? aarch64_is_long_call_p (callee) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ : !REG_P (callee)) XEXP (operands[0], 0) = force_reg (Pmode, callee); pat = gen_call_internal (operands[0], operands[1], operands[2]); aarch64_emit_call_insn (pat); DONE; }" ) (define_insn "*call_symbol" [(call (mem:DI (match_operand:DI 0 "" "")) (match_operand 1 "" "")) (use (match_operand 2 "" "")) (clobber (reg:DI LR_REGNUM))] "GET_CODE (operands[0]) == SYMBOL_REF && !aarch64_is_long_call_p (operands[0]) && !aarch64_is_noplt_call_p (operands[0])" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "bl\\t%a0" [(set_attr "type" "call")] ) (define_expand "call_value_internal" [(parallel [(set (match_operand 0 "" "") (call (match_operand 1 "memory_operand" "") (match_operand 2 "general_operand" ""))) (use (match_operand 3 "" "")) (clobber (reg:DI LR_REGNUM))])]) (define_expand "call_value" [(parallel [(set (match_operand 0 "" "") (call (match_operand 1 "memory_operand" "") (match_operand 2 "general_operand" ""))) (use (match_operand 3 "" "")) (clobber (reg:DI LR_REGNUM))])] "" " { rtx callee, pat; /* In an untyped call, we can get NULL for operand 3. */ if (operands[3] == NULL) operands[3] = const0_rtx; /* Decide if we should generate indirect calls by loading the 64-bit address of the callee into a register before performing the branch-and-link. */ callee = XEXP (operands[1], 0); if (GET_CODE (callee) == SYMBOL_REF ? aarch64_is_long_call_p (callee) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ : !REG_P (callee)) XEXP (operands[1], 0) = force_reg (Pmode, callee); pat = gen_call_value_internal (operands[0], operands[1], operands[2], operands[3]); aarch64_emit_call_insn (pat); DONE; }" ) (define_insn "*call_value_symbol" [(set (match_operand 0 "" "") (call (mem:DI (match_operand:DI 1 "" "")) (match_operand 2 "" ""))) (use (match_operand 3 "" "")) (clobber (reg:DI LR_REGNUM))] "GET_CODE (operands[1]) == SYMBOL_REF && !aarch64_is_long_call_p (operands[1]) && !aarch64_is_noplt_call_p (operands[1])" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "bl\\t%a1" [(set_attr "type" "call")] ) One of constraints is wrong. This patch fixes the ICE. -- H.J. --- diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index baa97fd..f7e871e 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -696,7 +696,8 @@ the branch-and-link. */ callee = XEXP (operands[0], 0); if (GET_CODE (callee) == SYMBOL_REF - ? aarch64_is_long_call_p (callee) + ? (aarch64_is_long_call_p (callee) + || aarch64_is_noplt_call_p (callee)) : !REG_P (callee)) XEXP (operands[0], 0) = force_reg (Pmode, callee); @@ -755,7 +756,8 @@ the branch-and-link. */ callee = XEXP (operands[1], 0); if (GET_CODE (callee) == SYMBOL_REF - ? aarch64_is_long_call_p (callee) + ? (aarch64_is_long_call_p (callee) + || aarch64_is_noplt_call_p (callee)) : !REG_P (callee)) XEXP (operands[1], 0) = force_reg (Pmode, callee);