diff mbox series

[1/2,ipa] Add target hook to sanitize cloned declaration's attributes

Message ID 78b498f6-b5bd-1a59-5ec7-1883e31ea633@arm.com
State New
Headers show
Series Fix cloning of 'cmse_nonsecure_entry' functions | expand

Commit Message

Andre Vieira (lists) Oct. 8, 2019, 3:20 p.m. UTC 
Hi,

This patch adds a new target hook called 
TARGET_HOOK_SANITIZE_CLONE_ATTRIBUTES.  This hook is meant to give each 
target the ability to sanitize a cloned's declaration attribute list.


Is this OK for trunk?

Cheers,
Andre

gcc/ChangeLog:

2019-10-08  Andre Vieira  <andre.simoesdiasvieira@arm.com>

	* ipa-cp.c(create_specialized_node): Call new target hook when
         creating a new node.
         * target.def(sanitize_clone_attributes): New target hook.
         * targhooks.c(default_sanitize_clone_attributes): New.
         * targhooks.h(default_sanitize_clone_attributes): New.
         * doc/tm.texi.in: Document new target hook.
         * doc/tm.texi: Regenerated

Comments

Martin Jambor Oct. 8, 2019, 4:27 p.m. UTC | #1 
Hi,

On Tue, Oct 08 2019, Andre Vieira (lists) wrote:
> Hi,
>
> This patch adds a new target hook called 
> TARGET_HOOK_SANITIZE_CLONE_ATTRIBUTES.  This hook is meant to give each 
> target the ability to sanitize a cloned's declaration attribute list.
>
>
> Is this OK for trunk?
>
> Cheers,
> Andre
>
> gcc/ChangeLog:
>
> 2019-10-08  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>
> 	* ipa-cp.c(create_specialized_node): Call new target hook when
>          creating a new node.

I'm quite sure you don't want to call the hook here but from within
cgraph_node::create_virtual_clone (and perhaps also
cgraph_node::create_version_clone_with_body?), if only now there are two
passes creating clones and there may easily be more in the future.

Thanks,

Martin

>          * target.def(sanitize_clone_attributes): New target hook.
>          * targhooks.c(default_sanitize_clone_attributes): New.
>          * targhooks.h(default_sanitize_clone_attributes): New.
>          * doc/tm.texi.in: Document new target hook.
>          * doc/tm.texi: Regenerated
Andre Vieira (lists) Oct. 9, 2019, 10:20 a.m. UTC | #2 
Hi Martin,

Thanks for the suggestion.  I have moved the target hook to the 
following functions:
'create_clone'
'create_version_clone'
'create_version_clone_with_body'

I looked for all 'cgraph_node::create_*' functions and added the target 
hook to them. The exception is 'create_virtual_clone' which calls 
'create_clone' so it should be caught there.

Is this OK for trunk?

Cheers,
Andre

gcc/ChangeLog:

2019-10-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>

	* cgraphclones.c(create_clone): Call new target hook when
         creating a new node.
         (create_version_clone): Likewise.
         (create_version_clone_with_body): Likewise.
         * target.def(sanitize_clone_attributes): New target hook.
         * targhooks.c(default_sanitize_clone_attributes): New.
         * targhooks.h(default_sanitize_clone_attributes): New.
         * doc/tm.texi.in: Document new target hook.
         * doc/tm.texi: Regenerated

On 08/10/2019 17:27, Martin Jambor wrote:
> Hi,
> 
> On Tue, Oct 08 2019, Andre Vieira (lists) wrote:
>> Hi,
>>
>> This patch adds a new target hook called
>> TARGET_HOOK_SANITIZE_CLONE_ATTRIBUTES.  This hook is meant to give each
>> target the ability to sanitize a cloned's declaration attribute list.
>>
>>
>> Is this OK for trunk?
>>
>> Cheers,
>> Andre
>>
>> gcc/ChangeLog:
>>
>> 2019-10-08  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>
>> 	* ipa-cp.c(create_specialized_node): Call new target hook when
>>           creating a new node.
> 
> I'm quite sure you don't want to call the hook here but from within
> cgraph_node::create_virtual_clone (and perhaps also
> cgraph_node::create_version_clone_with_body?), if only now there are two
> passes creating clones and there may easily be more in the future.
> 
> Thanks,
> 
> Martin
> 
>>           * target.def(sanitize_clone_attributes): New target hook.
>>           * targhooks.c(default_sanitize_clone_attributes): New.
>>           * targhooks.h(default_sanitize_clone_attributes): New.
>>           * doc/tm.texi.in: Document new target hook.
>>           * doc/tm.texi: Regenerated
Martin Jambor Oct. 9, 2019, 11:43 a.m. UTC | #3 
Hi,

On Wed, Oct 09 2019, Andre Vieira (lists) wrote:
> Hi Martin,
>
> Thanks for the suggestion.  I have moved the target hook to the 
> following functions:
> 'create_clone'
> 'create_version_clone'

I am not sure that calling the hook in create_version_clone is what you
want.  It should perhaps be made clearer in the API but unlike
(functions that call) create_clone and create_version_clone_with_body,
the result is not a local symbol but it may also be an externally
visible "version" of the original.  OpenMP uses it to create nodes for
external declarations (not definitions!) of variants of functions marked
with omp declare simd and the function calling it in trans-mem.c also
apparently uses it to create possibly externally visible clones.

So, if the idea of the patch is to allow stripping some attributes from
functions that are not externally visible - which I believe it is but I
may be wrong - you do not want to do that here.  Alternatively, the hook
itself could check DECL_EXTERNAL (node->decl).  Whether you decide to
invoke the call for non-local clones or not, please include it in the
hook description.

> 'create_version_clone_with_body'

The other two additions look OK to me.  You might want to check that the
attribute stripping works with LTO (but I tend to think that attributes
are streamed for WPA so it should be OK).

>
> I looked for all 'cgraph_node::create_*' functions and added the target 
> hook to them. The exception is 'create_virtual_clone' which calls 
> 'create_clone' so it should be caught there.
>
> Is this OK for trunk?

I'm afraid you'll need an ACK from a global reviewer for something like
this.

Martin


>
> Cheers,
> Andre
>
> gcc/ChangeLog:
>
> 2019-10-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>
> 	* cgraphclones.c(create_clone): Call new target hook when
>          creating a new node.
>          (create_version_clone): Likewise.
>          (create_version_clone_with_body): Likewise.
>          * target.def(sanitize_clone_attributes): New target hook.
>          * targhooks.c(default_sanitize_clone_attributes): New.
>          * targhooks.h(default_sanitize_clone_attributes): New.
>          * doc/tm.texi.in: Document new target hook.
>          * doc/tm.texi: Regenerated
Andre Vieira (lists) Oct. 9, 2019, 3:29 p.m. UTC | #4 
Hi Martin,

I see thank you. For my particular use case I only need to strip 
attributes for local symbols.  However, if I were to claim it is only 
used for non-externally visible clones than I would also need to make 
sure they are non-externally visible now and put something in place that 
checks this in the future.  I feel it's easier and more flexible to 
leave it up to the users of the hook to check it themselves using 
DECL_EXTERNAL.  I will mention it in the description though.

Cheers,
Andre

gcc/ChangeLog:

2019-10-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>

	* cgraphclones.c(create_clone): Call new target hook when
         creating a new node.
         (create_version_clone): Likewise.
         (create_version_clone_with_body): Likewise.
         * target.def(sanitize_clone_attributes): New target hook.
         * targhooks.c(default_sanitize_clone_attributes): New.
         * targhooks.h(default_sanitize_clone_attributes): New.
         * doc/tm.texi.in: Document new target hook.
         * doc/tm.texi: Regenerated

On 09/10/2019 12:43, Martin Jambor wrote:
> Hi,
> 
> On Wed, Oct 09 2019, Andre Vieira (lists) wrote:
>> Hi Martin,
>>
>> Thanks for the suggestion.  I have moved the target hook to the
>> following functions:
>> 'create_clone'
>> 'create_version_clone'
> 
> I am not sure that calling the hook in create_version_clone is what you
> want.  It should perhaps be made clearer in the API but unlike
> (functions that call) create_clone and create_version_clone_with_body,
> the result is not a local symbol but it may also be an externally
> visible "version" of the original.  OpenMP uses it to create nodes for
> external declarations (not definitions!) of variants of functions marked
> with omp declare simd and the function calling it in trans-mem.c also
> apparently uses it to create possibly externally visible clones.
Andre Vieira (lists) Oct. 9, 2019, 3:35 p.m. UTC | #5 
Adding Jakub to CC'

On 09/10/2019 16:29, Andre Vieira (lists) wrote:
> Hi Martin,
> 
> I see thank you. For my particular use case I only need to strip 
> attributes for local symbols.  However, if I were to claim it is only 
> used for non-externally visible clones than I would also need to make 
> sure they are non-externally visible now and put something in place that 
> checks this in the future.  I feel it's easier and more flexible to 
> leave it up to the users of the hook to check it themselves using 
> DECL_EXTERNAL.  I will mention it in the description though.
> 
> Cheers,
> Andre
> 
> gcc/ChangeLog:
> 
> 2019-10-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>
> 
>      * cgraphclones.c(create_clone): Call new target hook when
>          creating a new node.
>          (create_version_clone): Likewise.
>          (create_version_clone_with_body): Likewise.
>          * target.def(sanitize_clone_attributes): New target hook.
>          * targhooks.c(default_sanitize_clone_attributes): New.
>          * targhooks.h(default_sanitize_clone_attributes): New.
>          * doc/tm.texi.in: Document new target hook.
>          * doc/tm.texi: Regenerated
> 
> On 09/10/2019 12:43, Martin Jambor wrote:
>> Hi,
>>
>> On Wed, Oct 09 2019, Andre Vieira (lists) wrote:
>>> Hi Martin,
>>>
>>> Thanks for the suggestion.  I have moved the target hook to the
>>> following functions:
>>> 'create_clone'
>>> 'create_version_clone'
>>
>> I am not sure that calling the hook in create_version_clone is what you
>> want.  It should perhaps be made clearer in the API but unlike
>> (functions that call) create_clone and create_version_clone_with_body,
>> the result is not a local symbol but it may also be an externally
>> visible "version" of the original.  OpenMP uses it to create nodes for
>> external declarations (not definitions!) of variants of functions marked
>> with omp declare simd and the function calling it in trans-mem.c also
>> apparently uses it to create possibly externally visible clones.
>
Andre Vieira (lists) Oct. 15, 2019, 11:14 a.m. UTC | #6 
Hi,

I have changed the name to make the target hook more general and not 
create the illusion it is related to sanitizers.

Is this OK for trunk?

Cheers,
Andre

gcc/ChangeLog:

2019-10-15  Andre Vieira  <andre.simoesdiasvieira@arm.com>

	* cgraphclones.c(create_clone): Call new target hook when
         creating a new node.
         (create_version_clone_with_body): Likewise.
         * target.def(modify_clone_cgraph_node): New target hook.
         * targhooks.c(default_modify_clone_cgraph_node): New.
         * targhooks.h(default_modify_clone_cgraph_node): New.
         * doc/tm.texi.in: Include new TARGET_MODIFY_CLONE_CGRAPH_NODE.
         * doc/tm.texi: Regenerated

On 09/10/2019 16:35, Andre Vieira (lists) wrote:
> Adding Jakub to CC'
> 
> On 09/10/2019 16:29, Andre Vieira (lists) wrote:
>> Hi Martin,
>>
>> I see thank you. For my particular use case I only need to strip 
>> attributes for local symbols.  However, if I were to claim it is only 
>> used for non-externally visible clones than I would also need to make 
>> sure they are non-externally visible now and put something in place 
>> that checks this in the future.  I feel it's easier and more flexible 
>> to leave it up to the users of the hook to check it themselves using 
>> DECL_EXTERNAL.  I will mention it in the description though.
>>
>> Cheers,
>> Andre
>>
>> gcc/ChangeLog:
>>
>> 2019-10-09  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>
>>      * cgraphclones.c(create_clone): Call new target hook when
>>          creating a new node.
>>          (create_version_clone): Likewise.
>>          (create_version_clone_with_body): Likewise.
>>          * target.def(sanitize_clone_attributes): New target hook.
>>          * targhooks.c(default_sanitize_clone_attributes): New.
>>          * targhooks.h(default_sanitize_clone_attributes): New.
>>          * doc/tm.texi.in: Document new target hook.
>>          * doc/tm.texi: Regenerated
>>
>> On 09/10/2019 12:43, Martin Jambor wrote:
>>> Hi,
>>>
>>> On Wed, Oct 09 2019, Andre Vieira (lists) wrote:
>>>> Hi Martin,
>>>>
>>>> Thanks for the suggestion.  I have moved the target hook to the
>>>> following functions:
>>>> 'create_clone'
>>>> 'create_version_clone'
>>>
>>> I am not sure that calling the hook in create_version_clone is what you
>>> want.  It should perhaps be made clearer in the API but unlike
>>> (functions that call) create_clone and create_version_clone_with_body,
>>> the result is not a local symbol but it may also be an externally
>>> visible "version" of the original.  OpenMP uses it to create nodes for
>>> external declarations (not definitions!) of variants of functions marked
>>> with omp declare simd and the function calling it in trans-mem.c also
>>> apparently uses it to create possibly externally visible clones.
>>
Jan Hubicka Oct. 15, 2019, 11:58 a.m. UTC | #7 
> Hi,
> 
> I have changed the name to make the target hook more general and not create
> the illusion it is related to sanitizers.
> 
> Is this OK for trunk?
Can't you just go via IPA symbol summary or add_cgraph_duplication_hook?

Honza
diff mbox series

Patch

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index a86c210d4fe390bd0356b6e50ba7c6c34a36239a..5c1149ccf47576d8d803d1ab712f6f1b9342d74c 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -12039,6 +12039,11 @@  If defined, this function returns an appropriate alignment in bits for an atomic
 ISO C11 requires atomic compound assignments that may raise floating-point exceptions to raise exceptions corresponding to the arithmetic operation whose result was successfully stored in a compare-and-exchange sequence.  This requires code equivalent to calls to @code{feholdexcept}, @code{feclearexcept} and @code{feupdateenv} to be generated at appropriate points in the compare-and-exchange sequence.  This hook should set @code{*@var{hold}} to an expression equivalent to the call to @code{feholdexcept}, @code{*@var{clear}} to an expression equivalent to the call to @code{feclearexcept} and @code{*@var{update}} to an expression equivalent to the call to @code{feupdateenv}.  The three expressions are @code{NULL_TREE} on entry to the hook and may be left as @code{NULL_TREE} if no code is required in a particular place.  The default implementation leaves all three expressions as @code{NULL_TREE}.  The @code{__atomic_feraiseexcept} function from @code{libatomic} may be of use as part of the code generated in @code{*@var{update}}.
 @end deftypefn
 
+@deftypefn {Target Hook} void TARGET_SANITIZE_CLONE_ATTRIBUTES (struct cgraph_node *@var{})
+This target hook sanitizes the cloned @code{*@var{node}} declaration
+attributes.
+@end deftypefn
+
 @deftypefn {Target Hook} void TARGET_RECORD_OFFLOAD_SYMBOL (tree)
 Used when offloaded functions are seen in the compilation unit and no named
 sections are available.  It is called once for each symbol that must be
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 06dfcda35abea7396c288a59c38ee4ef57c6fef6..f4ef164a595ca1c2e286a7d26020b63b4c966728 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -8134,6 +8134,8 @@  and the associated definitions of those functions.
 
 @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 
+@hook TARGET_SANITIZE_CLONE_ATTRIBUTES
+
 @hook TARGET_RECORD_OFFLOAD_SYMBOL
 
 @hook TARGET_OFFLOAD_OPTIONS
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index b4fb74e097e2c9f80aa38f8e150be593200d9bdf..9c2154fa78d59873243fc7d7717fb9cab83101ac 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -124,6 +124,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-ccp.h"
 #include "stringpool.h"
 #include "attribs.h"
+#include "target.h"
 
 template <typename valtype> class ipcp_value;
 
@@ -4017,6 +4018,9 @@  create_specialized_node (struct cgraph_node *node,
   ipcp_discover_new_direct_edges (new_node, known_csts, known_contexts, aggvals);
 
   callers.release ();
+
+  targetm.sanitize_clone_attributes (new_node);
+
   return new_node;
 }
 
diff --git a/gcc/target.def b/gcc/target.def
index f9446fa05a22c79154c2ef36d3d8aea48a5efcc6..18301565e5131fa8b2f194bc73aa0c55d30cb9ef 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -6582,6 +6582,13 @@  DEFHOOK
  void, (tree *hold, tree *clear, tree *update),
  default_atomic_assign_expand_fenv)
 
+DEFHOOK
+(sanitize_clone_attributes,
+"This target hook sanitizes the cloned @code{*@var{node}} declaration\n\
+attributes.",
+void, (struct cgraph_node *),
+default_sanitize_clone_attributes)
+
 /* Leave the boolean fields at the end.  */
 
 /* True if we can create zeroed data by switching to a BSS section
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 5aba67660f85406b9fd475e75a3cc65b0d1952f5..0191d0ed36eb60fc79bf718b9782ef6c99c77fc4 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -284,4 +284,6 @@  extern rtx default_speculation_safe_value (machine_mode, rtx, rtx, rtx);
 extern void default_remove_extra_call_preserved_regs (rtx_insn *,
 						      HARD_REG_SET *);
 
+extern void default_sanitize_clone_attributes (struct cgraph_node *);
+
 #endif /* GCC_TARGHOOKS_H */
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index ed77afb1da57e59bc0725dc0d6fac477391bae03..35d2114956bd27f250335d918d0b3d81d962472b 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -2120,6 +2120,12 @@  default_atomic_assign_expand_fenv (tree *, tree *, tree *)
 {
 }
 
+/* Default implementation of TARGET_SANITIZE_CLONE_ATTRIBUTES.  */
+
+void default_sanitize_clone_attributes (struct cgraph_node *node)
+{
+}
+
 #ifndef PAD_VARARGS_DOWN
 #define PAD_VARARGS_DOWN BYTES_BIG_ENDIAN
 #endif