On Tue, Oct 27, 2015 at 10:44 AM, Jeff Law <l...@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.radhakrish...@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);
