PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

---------- Forwarded message ----------
From: H.J. Lu <hongjiu.lu@intel.com>
Date: Wed, Sep 9, 2015 at 3:02 PM
Subject: [PATCH] PR target/67215: -fno-plt needs improvements for x86
To: gcc-patches@gcc.gnu.org


prepare_call_address in calls.c is the wrong place to handle -fno-plt.
We shoudn't force function address into register and hope that load
function address via GOT and indirect call via register will be folded
into indirect call via GOT, which doesn't always happen.  Also non-PIC
case can only be handled in backend.  Instead, backend should expand
external function call into indirect call via GOT for -fno-plt.

This patch reverts -fno-plt in prepare_call_address and handles it in
ix86_expand_call.  Other backends may need similar changes to support
-fno-plt.  Alternately, we can introduce a target hook to indicate
whether an external function should be called via register for -fno-plt
so that i386 backend can disable it in prepare_call_address.

gcc/

        PR target/67215
        * calls.c (prepare_call_address): Don't handle -fno-plt here.
        * config/i386/i386.c (ix86_expand_call): Generate indirect call
        via GOT for -fno-plt.  Support indirect call via GOT for x32.
        * config/i386/predicates.md (sibcall_memory_operand): Allow
        GOT memory operand.

gcc/testsuite/

        PR target/67215
        * gcc.target/i386/pr67215-1.c: New test.
        * gcc.target/i386/pr67215-2.c: Likewise.
        * gcc.target/i386/pr67215-3.c: Likewise.
---
 gcc/calls.c                               | 12 ------
 gcc/config/i386/i386.c                    | 71 ++++++++++++++++++++++++-------
 gcc/config/i386/predicates.md             |  7 ++-
 gcc/testsuite/gcc.target/i386/pr67215-1.c | 20 +++++++++
 gcc/testsuite/gcc.target/i386/pr67215-2.c | 20 +++++++++
 gcc/testsuite/gcc.target/i386/pr67215-3.c | 13 ++++++
 6 files changed, 114 insertions(+), 29 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr67215-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr67215-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr67215-3.c

diff --git a/gcc/calls.c b/gcc/calls.c
index 026cb53..22c65cd 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -203,18 +203,6 @@ prepare_call_address (tree fndecl_or_type, rtx
funexp, rtx static_chain_value,
               && targetm.small_register_classes_for_mode_p (FUNCTION_MODE))
              ? force_not_mem (memory_address (FUNCTION_MODE, funexp))
              : memory_address (FUNCTION_MODE, funexp));
-  else if (flag_pic
-          && fndecl_or_type
-          && TREE_CODE (fndecl_or_type) == FUNCTION_DECL
-          && (!flag_plt
-              || lookup_attribute ("noplt", DECL_ATTRIBUTES (fndecl_or_type)))
-          && !targetm.binds_local_p (fndecl_or_type))
-    {
-      /* This is done only for PIC code.  There is no easy interface
to force the
-        function address into GOT for non-PIC case.  non-PIC case needs to be
-        handled specially by the backend.  */
-      funexp = force_reg (Pmode, funexp);
-    }
   else if (! sibcallp)
     {
       if (!NO_FUNCTION_CSE && optimize && ! flag_no_function_cse)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index d78f4e7..b9299d4 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -25649,21 +25649,54 @@ ix86_expand_call (rtx retval, rtx fnaddr,
rtx callarg1,
       /* Static functions and indirect calls don't need the pic
register.  Also,
         check if PLT was explicitly avoided via no-plt or "noplt"
attribute, making
         it an indirect call.  */
+      rtx addr = XEXP (fnaddr, 0);
       if (flag_pic
-         && (!TARGET_64BIT
-             || (ix86_cmodel == CM_LARGE_PIC
-                 && DEFAULT_ABI != MS_ABI))
-         && GET_CODE (XEXP (fnaddr, 0)) == SYMBOL_REF
-         && !SYMBOL_REF_LOCAL_P (XEXP (fnaddr, 0))
-         && flag_plt
-         && (SYMBOL_REF_DECL ((XEXP (fnaddr, 0))) == NULL_TREE
-             || !lookup_attribute ("noplt",
-                    DECL_ATTRIBUTES (SYMBOL_REF_DECL (XEXP (fnaddr, 0))))))
+         && GET_CODE (addr) == SYMBOL_REF
+         && !SYMBOL_REF_LOCAL_P (addr))
        {
-         use_reg (&use, gen_rtx_REG (Pmode, REAL_PIC_OFFSET_TABLE_REGNUM));
-         if (ix86_use_pseudo_pic_reg ())
-           emit_move_insn (gen_rtx_REG (Pmode, REAL_PIC_OFFSET_TABLE_REGNUM),
-                           pic_offset_table_rtx);
+         if (flag_plt
+             && (SYMBOL_REF_DECL (addr) == NULL_TREE
+                 || !lookup_attribute ("noplt",
+                                       DECL_ATTRIBUTES
(SYMBOL_REF_DECL (addr)))))
+           {
+             if (!TARGET_64BIT
+                 || (ix86_cmodel == CM_LARGE_PIC
+                     && DEFAULT_ABI != MS_ABI))
+               {
+                 use_reg (&use, gen_rtx_REG (Pmode,
+                                             REAL_PIC_OFFSET_TABLE_REGNUM));
+                 if (ix86_use_pseudo_pic_reg ())
+                   emit_move_insn (gen_rtx_REG (Pmode,
+                                                REAL_PIC_OFFSET_TABLE_REGNUM),
+                                   pic_offset_table_rtx);
+               }
+           }
+         else if (!TARGET_PECOFF && !TARGET_MACHO)
+           {
+             if (TARGET_64BIT)
+               {
+                 fnaddr = gen_rtx_UNSPEC (Pmode,
+                                          gen_rtvec (1, addr),
+                                          UNSPEC_GOTPCREL);
+                 fnaddr = gen_rtx_CONST (Pmode, fnaddr);
+               }
+             else
+               {
+                 fnaddr = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, addr),
+                                          UNSPEC_GOT);
+                 fnaddr = gen_rtx_CONST (Pmode, fnaddr);
+                 fnaddr = gen_rtx_PLUS (Pmode, pic_offset_table_rtx,
+                                        fnaddr);
+               }
+             fnaddr = gen_const_mem (Pmode, fnaddr);
+             /* Pmode may not be the same as word_mode for x32, which
+                doesn't support indirect branch via 32-bit memory slot.
+                Since x32 GOT slot is 64 bit with zero upper 32 bits,
+                indirect branch via x32 GOT slot is OK.  */
+             if (GET_MODE (fnaddr) != word_mode)
+               fnaddr = gen_rtx_ZERO_EXTEND (word_mode, fnaddr);
+             fnaddr = gen_rtx_MEM (QImode, fnaddr);
+           }
        }
     }

@@ -25685,9 +25718,15 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx callarg1,
       && GET_CODE (XEXP (fnaddr, 0)) == SYMBOL_REF
       && !local_symbolic_operand (XEXP (fnaddr, 0), VOIDmode))
     fnaddr = gen_rtx_MEM (QImode, construct_plt_address (XEXP (fnaddr, 0)));
-  else if (sibcall
-          ? !sibcall_insn_operand (XEXP (fnaddr, 0), word_mode)
-          : !call_insn_operand (XEXP (fnaddr, 0), word_mode))
+  /* Since x32 GOT slot is 64 bit with zero upper 32 bits, indirect
+     branch via x32 GOT slot is OK.  */
+  else if (!(TARGET_X32
+            && MEM_P (fnaddr)
+            && GET_CODE (XEXP (fnaddr, 0)) == ZERO_EXTEND
+            && GOT_memory_operand (XEXP (XEXP (fnaddr, 0), 0), Pmode))
+          && (sibcall
+              ? !sibcall_insn_operand (XEXP (fnaddr, 0), word_mode)
+              : !call_insn_operand (XEXP (fnaddr, 0), word_mode)))
     {
       fnaddr = convert_to_mode (word_mode, XEXP (fnaddr, 0), 1);
       fnaddr = gen_rtx_MEM (QImode, copy_to_mode_reg (word_mode, fnaddr));
diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
index bc76a5b..822d834 100644
--- a/gcc/config/i386/predicates.md
+++ b/gcc/config/i386/predicates.md
@@ -593,7 +593,12 @@
 ;; Return true if OP is a memory operands that can be used in sibcalls.
 (define_predicate "sibcall_memory_operand"
   (and (match_operand 0 "memory_operand")
-       (match_test "CONSTANT_P (XEXP (op, 0))")))
+       (match_test "CONSTANT_P (XEXP (op, 0))
+                   || (GET_CODE (XEXP (op, 0)) == PLUS
+                       && REG_P (XEXP (XEXP (op, 0), 0))
+                       && GET_CODE (XEXP (XEXP (op, 0), 1)) == CONST
+                       && GET_CODE (XEXP (XEXP (XEXP (op, 0), 1), 0)) == UNSPEC
+                       && XINT (XEXP (XEXP (XEXP (op, 0), 1), 0), 1)
== UNSPEC_GOT)")))

 ;; Test for a valid operand for a call instruction.
 ;; Allow constant call address operands in Pmode only.
diff --git a/gcc/testsuite/gcc.target/i386/pr67215-1.c
b/gcc/testsuite/gcc.target/i386/pr67215-1.c
new file mode 100644
index 0000000..fd37f8e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr67215-1.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic -fno-plt" } */
+
+extern char* bar (int);
+extern char* arr[32];
+
+void
+foo (void)
+{
+  int i;
+
+  for (i = 0; i < 32; i++)
+    arr[i] = bar (128);
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*.bar@GOTPCREL" { target {
! ia32 } } } } */
+/* { dg-final { scan-assembler "call\[ \t\]*.bar@GOT\\(" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "mov(l|q)\[ \t\]*.bar@GOTPCREL" {
target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]*.bar@GOT\\(" { target
ia32 } } } */
+/* { dg-final { scan-assembler-not "call\[ \t\]*.bar@PLT" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr67215-2.c
b/gcc/testsuite/gcc.target/i386/pr67215-2.c
new file mode 100644
index 0000000..ebf2919
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr67215-2.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+extern char* bar (int) __attribute__ ((noplt));
+extern char* arr[32];
+
+void
+foo (void)
+{
+  int i;
+
+  for (i = 0; i < 32; i++)
+    arr[i] = bar (128);
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*.bar@GOTPCREL" { target {
! ia32 } } } } */
+/* { dg-final { scan-assembler "call\[ \t\]*.bar@GOT\\(" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "mov(l|q)\[ \t\]*.bar@GOTPCREL" {
target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]*.bar@GOT\\(" { target
ia32 } } } */
+/* { dg-final { scan-assembler-not "call\[ \t\]*.bar@PLT" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr67215-3.c
b/gcc/testsuite/gcc.target/i386/pr67215-3.c
new file mode 100644
index 0000000..eb6bb39
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr67215-3.c
@@ -0,0 +1,13 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic -fno-plt -fdump-rtl-expand" } */
+
+extern int bar (void);
+
+int
+foo (void)
+{
+  return bar ();
+}
+
+/* { dg-final { scan-rtl-dump "\\(call \\(mem:QI \\(mem/u/c:"
"expand" { target { ! x32 } } } } */
+/* { dg-final { scan-rtl-dump "\\(call \\(mem:QI \\(zero_extend:DI
\\(mem/u/c:" "expand" { target x32 } } } */
--
2.4.3



-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Bernd Schmidt]

On 10/19/2015 09:55 PM, H.J. Lu wrote:
>          * calls.c (prepare_call_address): Don't handle -fno-plt here.

Is any other target using -fno-plt? If not, and if that's really just a 
revert I'll approve it on the condition that the x86 maintainers are 
happy with the rest of the changes.

Your patch is word-wrapped.


Bernd

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Tue, Oct 27, 2015 at 4:20 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/19/2015 09:55 PM, H.J. Lu wrote:
>>
>>          * calls.c (prepare_call_address): Don't handle -fno-plt here.
>
>
> Is any other target using -fno-plt? If not, and if that's really just a

aarch64 is the only target which checks -fno-plt from commit log below.
I CCed the code author.  aarch64 may suffer from the same issue.

> revert I'll approve it on the condition that the x86 maintainers are happy
> with the rest of the changes.

Uros, can you take a look?

> Your patch is word-wrapped.

Sorry for that.  Here is the link for the patch:

https://gcc.gnu.org/git/?p=gcc.git;a=patch;h=a116a4af073bb99c3117dae38f02d528815bc431

Thanks.

-- 
H.J.
---
Author: jiwang <jiwang@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Aug 6 16:02:16 2015 +0000

    [AArch64] Tighten direct call pattern for sibcall to repair -fno-plt

    2015-08-06  Jiong Wang  <jiong.wang@arm.com>

    gcc/
      * config/aarch64/constraints.md (Usf): Add the test of
      aarch64_is_noplt_call_p.

    gcc/testsuite/
      * gcc.target/aarch64/noplt_3.c: New testcase.



    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@226682
138bc75d-0d04-0410-961f-82ee72b054a4

commit 2bcb7473b37f9aa76e530f0a2007893489f61586
Author: jiwang <jiwang@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Aug 6 15:57:36 2015 +0000

    [AArch64] Tighten direct call pattern to repair -fno-plt

    2015-08-06  Jiong Wang  <jiong.wang@arm.com>

    gcc/
      * config/aarch64/aarch64-protos.h (aarch64_is_noplt_call_p): New
declaration.
      * config/aarch64/aarch64.c (aarch64_is_noplt_call_p): New function.
      * config/aarch64/aarch64.md (call_value_symbol): Check noplt scenarios.
      (call_symbol): Likewise.

    gcc/testsuite/
      * gcc.target/aarch64/noplt_1.c: New testcase.
      * gcc.target/aarch64/noplt_2.c: Likewise.



    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@226681
138bc75d-0d04-0410-961f-82ee72b054a4

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Uros Bizjak]

On Tue, Oct 27, 2015 at 12:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 27, 2015 at 4:20 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/19/2015 09:55 PM, H.J. Lu wrote:
>>>
>>>          * calls.c (prepare_call_address): Don't handle -fno-plt here.
>>
>>
>> Is any other target using -fno-plt? If not, and if that's really just a
>
> aarch64 is the only target which checks -fno-plt from commit log below.
> I CCed the code author.  aarch64 may suffer from the same issue.
>
>> revert I'll approve it on the condition that the x86 maintainers are happy
>> with the rest of the changes.
>
> Uros, can you take a look?

I can only say that I don't oppose the changes. The details of linking
are out of my expertise, but OTOH, you probably know these details
better than I.

So, I'll rubberstamp the patch with OK, assuming you will fix eventual fallout.

Thanks,
Uros.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jiong Wang]

On 27/10/15 11:37, H.J. Lu wrote:
> On Tue, Oct 27, 2015 at 4:20 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/19/2015 09:55 PM, H.J. Lu wrote:
>>>           * calls.c (prepare_call_address): Don't handle -fno-plt here.
>>
>> Is any other target using -fno-plt? If not, and if that's really just a
> aarch64 is the only target which checks -fno-plt from commit log below.
> I CCed the code author.  aarch64 may suffer from the same issue.

H.J,

   Thanks for the info.

   After a quick reading of the PR at 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67215,
   I found the problem description is very x86 specific, I still don't 
understand what's
   wrong if we do the following transformation when -fno-plt specified 
on x86-74, thus I
   am not sure whether there is problem exist on aarch64. Can you please 
give more explanation?

   call    proc2@PLT

   ||
   V

   movq    proc2@GOTPCREL(%rip), %rax
   call    *%rax

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Tue, Oct 27, 2015 at 5:52 AM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
>
>
> On 27/10/15 11:37, H.J. Lu wrote:
>>
>> On Tue, Oct 27, 2015 at 4:20 AM, Bernd Schmidt <bschmidt@redhat.com>
>> wrote:
>>>
>>> On 10/19/2015 09:55 PM, H.J. Lu wrote:
>>>>
>>>>           * calls.c (prepare_call_address): Don't handle -fno-plt here.
>>>
>>>
>>> Is any other target using -fno-plt? If not, and if that's really just a
>>
>> aarch64 is the only target which checks -fno-plt from commit log below.
>> I CCed the code author.  aarch64 may suffer from the same issue.
>
>
> H.J,
>
>   Thanks for the info.
>
>   After a quick reading of the PR at
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67215,
>   I found the problem description is very x86 specific, I still don't
> understand what's
>   wrong if we do the following transformation when -fno-plt specified on
> x86-74, thus I
>   am not sure whether there is problem exist on aarch64. Can you please give
> more explanation?
>
>   call    proc2@PLT
>
>   ||
>   V
>
>   movq    proc2@GOTPCREL(%rip), %rax
>   call    *%rax

call *proc2@GOTPCREL(%rip)

doesn't use a register and saves one instruction.

-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jiong Wang]

On 27/10/15 13:06, H.J. Lu wrote:
> On Tue, Oct 27, 2015 at 5:52 AM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
>>
>> On 27/10/15 11:37, H.J. Lu wrote:
>>> On Tue, Oct 27, 2015 at 4:20 AM, Bernd Schmidt <bschmidt@redhat.com>
>>> wrote:
>>>> On 10/19/2015 09:55 PM, H.J. Lu wrote:
>>>>>            * calls.c (prepare_call_address): Don't handle -fno-plt here.
>>>>
>>>> Is any other target using -fno-plt? If not, and if that's really just a
>>> aarch64 is the only target which checks -fno-plt from commit log below.
>>> I CCed the code author.  aarch64 may suffer from the same issue.
>>
>> H.J,
>>
>>    Thanks for the info.
>>
>>    After a quick reading of the PR at
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67215,
>>    I found the problem description is very x86 specific, I still don't
>> understand what's
>>    wrong if we do the following transformation when -fno-plt specified on
>> x86-74, thus I
>>    am not sure whether there is problem exist on aarch64. Can you please give
>> more explanation?
>>
>>    call    proc2@PLT
>>
>>    ||
>>    V
>>
>>    movq    proc2@GOTPCREL(%rip), %rax
>>    call    *%rax
> call *proc2@GOTPCREL(%rip)
>
> doesn't use a register and saves one instruction.

OK, then it's fairly x86-64 specific optimization, because we can't do 
"call *mem" in
aarch64 and some other targets.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Ramana Radhakrishnan]

> 
> 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))


regards
Ramana

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan
<ramana.radhakrishnan@foss.arm.com> 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.

-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jiong Wang]

On 27/10/15 14:50, H.J. Lu wrote:
> On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan
> <ramana.radhakrishnan@foss.arm.com> 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.
>

The removing of those generic code has broken aarch64.

Actually those code in calls.c shouldn't prevent such "call *mem" 
opportunity on x86-64 because the combine pass
should combine "load reg, symbol + call reg" back into "call *mem" on 
x86-64 as there is related define_insn.

the testcases in PR67215 and included in your patch, all of which are 
loops, failed because either RTL PRE or loop pass will
hoist address calculation pattern as invariant out of loop into another 
basic block different with the call_insn. while combine
pass only work within basic block scope, thus we have missed such 
combine opportunity on x86-64.

I am not sure anyone has done experiment before on extend combine pass 
to larger scope.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Tue, Oct 27, 2015 at 8:26 AM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
>
>
> On 27/10/15 14:50, H.J. Lu wrote:
>>
>> On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan
>> <ramana.radhakrishnan@foss.arm.com> 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.
>>
>
> The removing of those generic code has broken aarch64.
>

Can you handle -fno-plt in aarch64 call expander?

-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Ramana Radhakrishnan]

On 27/10/15 14:50, H.J. Lu wrote:
> On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan
> <ramana.radhakrishnan@foss.arm.com> 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 - As I suspected the handling in the backend isn't sufficient, the call expanders need to handle this case in the AArch64 backend. Minimally tested -

Ok if no regressions on aarch64-none-elf?

regards
Ramana

* config/aarch64/aarch64.md (call, call_value): Handle noplt.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

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
>> <ramana.radhakrishnan@foss.arm.com> 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 <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
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?

Jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

On 10/27/2015 09:26 AM, Jiong Wang wrote:
>
>
> On 27/10/15 14:50, H.J. Lu wrote:
>> On Tue, Oct 27, 2015 at 7:34 AM, Ramana Radhakrishnan
>> <ramana.radhakrishnan@foss.arm.com> 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.
>>
>
> The removing of those generic code has broken aarch64.
>
> Actually those code in calls.c shouldn't prevent such "call *mem"
> opportunity on x86-64 because the combine pass
> should combine "load reg, symbol + call reg" back into "call *mem" on
> x86-64 as there is related define_insn.
I'd like to know why that wasn't happening.  That seems like the natural 
way for this to work.

>
> the testcases in PR67215 and included in your patch, all of which are
> loops, failed because either RTL PRE or loop pass will
> hoist address calculation pattern as invariant out of loop into another
> basic block different with the call_insn. while combine
> pass only work within basic block scope, thus we have missed such
> combine opportunity on x86-64.
>
> I am not sure anyone has done experiment before on extend combine pass
> to larger scope.
There was a group that poked at multi-basic-block combining and 
discussed at the Cauldron (California, can't recall which year that 
was).  I'm not sure if that work is active anymore.

Jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Tue, Oct 27, 2015 at 10:44 AM, Jeff Law <law@redhat.com> 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
>>> <ramana.radhakrishnan@foss.arm.com> 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 <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
> 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);

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

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
>> <ramana.radhakrishnan@foss.arm.com> 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 - As I suspected the handling in the backend isn't sufficient, the call expanders need to handle this case in the AArch64 backend. Minimally tested -
>
> Ok if no regressions on aarch64-none-elf?
>
> regards
> Ramana
>
> * config/aarch64/aarch64.md (call, call_value): Handle noplt.
FWIW -ENOPATCH.

jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Ramana Radhakrishnan]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]



On 27/10/15 20:57, Jeff Law wrote:
>> a
>>
>> * config/aarch64/aarch64.md (call, call_value): Handle noplt.
> FWIW -ENOPATCH.
> 
> jeff


Bah - finger trouble. Sorry about that. Here it is and also handling sibcall patterns. Tested aarch64-none-elf with no regressions.



2015-10-28  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

        * config/aarch64/aarch64.md (call_value, call, sibcall_value,
        sibcall): Handle noplt.

[-- Attachment #2: noplt-aarch64.txt --]
[-- Type: text/plain, Size: 1929 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index baa97fd..1bc6237 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);
 
@@ -805,10 +807,12 @@
   ""
   {
     rtx pat;
-
-    if (!REG_P (XEXP (operands[0], 0))
-       && (GET_CODE (XEXP (operands[0], 0)) != SYMBOL_REF))
-     XEXP (operands[0], 0) = force_reg (Pmode, XEXP (operands[0], 0));
+    rtx callee = XEXP (operands[0], 0);
+    if (!REG_P (callee)
+       && ((GET_CODE (callee) != SYMBOL_REF)
+	   || (GET_CODE (callee) == SYMBOL_REF
+	      && aarch64_is_noplt_call_p (callee))))
+      XEXP (operands[0], 0) = force_reg (Pmode, callee);
 
     if (operands[2] == NULL_RTX)
       operands[2] = const0_rtx;
@@ -835,10 +839,12 @@
   ""
   {
     rtx pat;
-
-    if (!REG_P (XEXP (operands[1], 0))
-       && (GET_CODE (XEXP (operands[1], 0)) != SYMBOL_REF))
-     XEXP (operands[1], 0) = force_reg (Pmode, XEXP (operands[1], 0));
+    rtx callee = XEXP (operands[1], 0);
+    if (!REG_P (callee)
+       && ((GET_CODE (callee) != SYMBOL_REF)
+	   || (GET_CODE (callee) == SYMBOL_REF
+	      && aarch64_is_noplt_call_p (callee))))
+      XEXP (operands[1], 0) = force_reg (Pmode, callee);
 
     if (operands[3] == NULL_RTX)
       operands[3] = const0_rtx;

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[James Greenhalgh]

On Wed, Oct 28, 2015 at 10:13:07AM +0000, Ramana Radhakrishnan wrote:
> 
> 
> On 27/10/15 20:57, Jeff Law wrote:
> >> a
> >>
> >> * config/aarch64/aarch64.md (call, call_value): Handle noplt.
> > FWIW -ENOPATCH.
> > 
> > jeff
> 
> 
> Bah - finger trouble. Sorry about that. Here it is and also handling sibcall
> patterns. Tested aarch64-none-elf with no regressions.

I think there is a small arithmetic simplification possible here...


> 2015-10-28  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
> 
>         * config/aarch64/aarch64.md (call_value, call, sibcall_value,
>         sibcall): Handle noplt.

> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index baa97fd..1bc6237 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);
>  

These hunks are fine.

> @@ -805,10 +807,12 @@
>    ""
>    {
>      rtx pat;
> -
> -    if (!REG_P (XEXP (operands[0], 0))
> -       && (GET_CODE (XEXP (operands[0], 0)) != SYMBOL_REF))
> -     XEXP (operands[0], 0) = force_reg (Pmode, XEXP (operands[0], 0));
> +    rtx callee = XEXP (operands[0], 0);

> +    if (!REG_P (callee)
> +       && ((GET_CODE (callee) != SYMBOL_REF)
> +	   || (GET_CODE (callee) == SYMBOL_REF
> +	      && aarch64_is_noplt_call_p (callee))))

    if (!REG_P (callee)
       && ((GET_CODE (callee) != SYMBOL_REF)
	   || && aarch64_is_noplt_call_p (callee)))

???

> @@ -835,10 +839,12 @@
>    ""
>    {
>      rtx pat;
> -
> -    if (!REG_P (XEXP (operands[1], 0))
> -       && (GET_CODE (XEXP (operands[1], 0)) != SYMBOL_REF))
> -     XEXP (operands[1], 0) = force_reg (Pmode, XEXP (operands[1], 0));
> +    rtx callee = XEXP (operands[1], 0);
> +    if (!REG_P (callee)
> +       && ((GET_CODE (callee) != SYMBOL_REF)
> +	   || (GET_CODE (callee) == SYMBOL_REF
> +	      && aarch64_is_noplt_call_p (callee))))
> +      XEXP (operands[1], 0) = force_reg (Pmode, callee);

Likewise.

Thanks,
James

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[James Greenhalgh]

On Wed, Oct 28, 2015 at 11:01:15AM +0000, James Greenhalgh wrote:
> On Wed, Oct 28, 2015 at 10:13:07AM +0000, Ramana Radhakrishnan wrote:
> > 
> > 
> > On 27/10/15 20:57, Jeff Law wrote:
> > >> a
> > >>
> > >> * config/aarch64/aarch64.md (call, call_value): Handle noplt.
> > > FWIW -ENOPATCH.
> > > 
> > > jeff
> > 
> > 
> > Bah - finger trouble. Sorry about that. Here it is and also handling sibcall
> > patterns. Tested aarch64-none-elf with no regressions.
> 
> 
>     if (!REG_P (callee)
>        && ((GET_CODE (callee) != SYMBOL_REF)
> 	   || && aarch64_is_noplt_call_p (callee)))
> 
> ???

This thread seems destined to cause typos and finger slips...

     if (!REG_P (callee)
        && ((GET_CODE (callee) != SYMBOL_REF)
 	     || aarch64_is_noplt_call_p (callee)))

Obviously :).

Thanks,
James

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Ramana Radhakrishnan]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]


> 
> This thread seems destined to cause typos and finger slips...
> 
>      if (!REG_P (callee)
>         && ((GET_CODE (callee) != SYMBOL_REF)
>  	     || aarch64_is_noplt_call_p (callee)))
> 
> Obviously :).

Sigh, Yeah it is one of those patches. Applied to trunk with the changes.

Ramana

> 
> Thanks,
> James
> 

[-- Attachment #2: noplt-aarch64.txt --]
[-- Type: text/plain, Size: 1872 bytes --]

Index: gcc/config/aarch64/aarch64.md
===================================================================
--- gcc/config/aarch64/aarch64.md	(revision 229482)
+++ gcc/config/aarch64/aarch64.md	(working copy)
@@ -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);
 
@@ -805,11 +807,12 @@
   ""
   {
     rtx pat;
+    rtx callee = XEXP (operands[0], 0);
+    if (!REG_P (callee)
+       && ((GET_CODE (callee) != SYMBOL_REF)
+	   || aarch64_is_noplt_call_p (callee)))
+      XEXP (operands[0], 0) = force_reg (Pmode, callee);
 
-    if (!REG_P (XEXP (operands[0], 0))
-       && (GET_CODE (XEXP (operands[0], 0)) != SYMBOL_REF))
-     XEXP (operands[0], 0) = force_reg (Pmode, XEXP (operands[0], 0));
-
     if (operands[2] == NULL_RTX)
       operands[2] = const0_rtx;
 
@@ -835,11 +838,12 @@
   ""
   {
     rtx pat;
+    rtx callee = XEXP (operands[1], 0);
+    if (!REG_P (callee)
+       && ((GET_CODE (callee) != SYMBOL_REF)
+	   || aarch64_is_noplt_call_p (callee)))
+      XEXP (operands[1], 0) = force_reg (Pmode, callee);
 
-    if (!REG_P (XEXP (operands[1], 0))
-       && (GET_CODE (XEXP (operands[1], 0)) != SYMBOL_REF))
-     XEXP (operands[1], 0) = force_reg (Pmode, XEXP (operands[1], 0));
-
     if (operands[3] == NULL_RTX)
       operands[3] = const0_rtx;
 

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

On 10/27/2015 12:54 PM, H.J. Lu wrote:
>>> 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 <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
>> 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.
You didn't answer my question.

I asked why you committed a patch given it didn't meet the  conditions 
Bernd set forth for approval.  I didn't ask anything about the bug itself.

So I'll ask again, why did you commit a patch which you clearly knew did 
not meet the conditions Bernd set forth for approval?

Jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
> On 10/27/2015 12:54 PM, H.J. Lu wrote:
>>>>
>>>> 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 <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
>>> 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.
>
> You didn't answer my question.
>
> I asked why you committed a patch given it didn't meet the  conditions Bernd
> set forth for approval.  I didn't ask anything about the bug itself.
>
> So I'll ask again, why did you commit a patch which you clearly knew did not
> meet the conditions Bernd set forth for approval?

I believed that aarch64 backend didn't properly handle -fno-plt,
which shouldn't block my patch.


-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Bernd Schmidt]

On 10/29/2015 02:10 AM, H.J. Lu wrote:
> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>
>> So I'll ask again, why did you commit a patch which you clearly knew did not
>> meet the conditions Bernd set forth for approval?
>
> I believed that aarch64 backend didn't properly handle -fno-plt,
> which shouldn't block my patch.

This really isn't how the rules work, and you've been around long enough 
to know it.


Bernd

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>
>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>>
>>> So I'll ask again, why did you commit a patch which you clearly knew did
>>> not
>>> meet the conditions Bernd set forth for approval?
>>
>>
>> I believed that aarch64 backend didn't properly handle -fno-plt,
>> which shouldn't block my patch.
>
>
> This really isn't how the rules work, and you've been around long enough to
> know it.
>

Sometimes It seems that it is the only way to get attention from the
community.  BTW, my patch was submitted in August.

-- 
H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Bernd Schmidt]

On 10/29/2015 02:14 AM, H.J. Lu wrote:
> On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>>
>>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>>
>>>>
>>>> So I'll ask again, why did you commit a patch which you clearly knew did
>>>> not
>>>> meet the conditions Bernd set forth for approval?
>>>
>>>
>>> I believed that aarch64 backend didn't properly handle -fno-plt,
>>> which shouldn't block my patch.

Actually this is even worse than I thought because it sounds like you're 
saying you knowingly checked something in while being aware it would 
break another port.

>> This really isn't how the rules work, and you've been around long enough to
>> know it.
>>
>
> Sometimes It seems that it is the only way to get attention from the
> community.  BTW, my patch was submitted in August.

At the point where you committed the patch you already had attention and 
the patch was being discussed.


Bernd

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[H.J. Lu]

On Wed, Oct 28, 2015 at 6:21 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/29/2015 02:14 AM, H.J. Lu wrote:
>>
>> On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com>
>> wrote:
>>>
>>> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> So I'll ask again, why did you commit a patch which you clearly knew
>>>>> did
>>>>> not
>>>>> meet the conditions Bernd set forth for approval?
>>>>
>>>>
>>>>
>>>> I believed that aarch64 backend didn't properly handle -fno-plt,
>>>> which shouldn't block my patch.
>
>
> Actually this is even worse than I thought because it sounds like you're
> saying you knowingly checked something in while being aware it would break
> another port.
>

Only when -fno-plt was used.

H.J.

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Ramana Radhakrishnan]

On 29/10/15 01:47, H.J. Lu wrote:
> On Wed, Oct 28, 2015 at 6:21 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/29/2015 02:14 AM, H.J. Lu wrote:
>>>
>>> On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com>
>>> wrote:
>>>>
>>>> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>>>>
>>>>>
>>>>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> So I'll ask again, why did you commit a patch which you clearly knew
>>>>>> did
>>>>>> not
>>>>>> meet the conditions Bernd set forth for approval?
>>>>>
>>>>>
>>>>>
>>>>> I believed that aarch64 backend didn't properly handle -fno-plt,
>>>>> which shouldn't block my patch.
>>
>>
>> Actually this is even worse than I thought because it sounds like you're
>> saying you knowingly checked something in while being aware it would break
>> another port.
>>
> 
> Only when -fno-plt was used.

So, that's a target independent feature in GCC ! So, I don't see how that
justifies the commit.


>> 
>> Sometimes It seems that it is the only way to get attention from the
>> community.  BTW, my patch was submitted in August.
>> 

I personally do *not* understand how that is an excuse. 

regards
Ramana


 

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

On 10/28/2015 07:10 PM, H.J. Lu wrote:
>> You didn't answer my question.
>>
>> I asked why you committed a patch given it didn't meet the  conditions Bernd
>> set forth for approval.  I didn't ask anything about the bug itself.
>>
>> So I'll ask again, why did you commit a patch which you clearly knew did not
>> meet the conditions Bernd set forth for approval?
>
> I believed that aarch64 backend didn't properly handle -fno-plt,
> which shouldn't block my patch.
Speaking strictly for myself at the moment...

--

You ought to know better than that.  A conditional approval was given, 
but your patch did not meet the conditions and thus it can not be 
considered approved.

At that point you could have *asked* if your patch could go forward, or 
worked with the AArch64 maintainers (who are very responsive) to reach a 
resolution and resubmitted a joint patch.

Instead you knowingly committed an unapproved patch.

Conditional approvals are a tool reviewers can use to help move patches 
along a little faster as are commit privileges.  Both rely on a level of 
trust that the reviewers and project as a whole extends to the 
contributor, namely that the contributor will only commit approved 
patches.  If a contributor can't be trusted to follow that rule, then 
we've got a serious problem.

--


Jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

On 10/28/2015 07:14 PM, H.J. Lu wrote:
> On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>>
>>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>>
>>>>
>>>> So I'll ask again, why did you commit a patch which you clearly knew did
>>>> not
>>>> meet the conditions Bernd set forth for approval?
>>>
>>>
>>> I believed that aarch64 backend didn't properly handle -fno-plt,
>>> which shouldn't block my patch.
>>
>>
>> This really isn't how the rules work, and you've been around long enough to
>> know it.
>>
>
> Sometimes It seems that it is the only way to get attention from the
> community.  BTW, my patch was submitted in August.
Again, speaking strictly for myself.

--

That's unacceptable behaviour on your part.  It doesn't matter if your 
patch was submitted 25+ years ago, yesterday or somewhere in between.

Other folks ping, wait and find a way to work with the reviewers.  Your 
solution was to violate the basic rules set for for commit privileges. 
  What makes you special that allows you to break the rules?

In my mind that kind of behaviour is anti-social and can't be tolerated 
in this community.

--

jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[Jeff Law]

On 10/29/2015 03:25 AM, Ramana Radhakrishnan wrote:
>>> Actually this is even worse than I thought because it sounds like you're
>>> saying you knowingly checked something in while being aware it would break
>>> another port.
>>>
>>
>> Only when -fno-plt was used.
>
> So, that's a target independent feature in GCC ! So, I don't see how that
> justifies the commit.
>
>
>>>
>>> Sometimes It seems that it is the only way to get attention from the
>>> community.  BTW, my patch was submitted in August.
>>>
>
> I personally do *not* understand how that is an excuse.
Likewise for both.


jeff

Re: PING: [PATCH] PR target/67215: -fno-plt needs improvements for x86

[David Edelsohn]

On Wed, 28 Oct 2015 at 18:14 PM, H.J. Lu wrote:
> On Wed, Oct 28, 2015 at 6:11 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 10/29/2015 02:10 AM, H.J. Lu wrote:
>>>
>>> On Wed, Oct 28, 2015 at 5:23 PM, Jeff Law <law@redhat.com> wrote:
>>>>
>>>>
>>>> So I'll ask again, why did you commit a patch which you clearly knew did
>>>> not
>>>> meet the conditions Bernd set forth for approval?
>>>
>>>
>>> I believed that aarch64 backend didn't properly handle -fno-plt,
>>> which shouldn't block my patch.
>>
>>
>> This really isn't how the rules work, and you've been around long enough to
>> know it.
>>
>
> Sometimes It seems that it is the only way to get attention from the
> community.  BTW, my patch was submitted in August.

H.J.:

Because you have committed unapproved patches on several occasions,
contrary to specific requests from GCC reviewers, the GCC Steering
Committee has voted to suspend your committer privileges to the GCC
Repository for two weeks. Future unapproved commits will lead to
longer suspensions.

- The GCC Steering Committee