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?

Jeff

Reply via email to