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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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