public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc(refs/vendors/ARM/heads/morello)] Handle MEM_REF[&<something>] in gimple
@ 2022-11-22 12:43 Stam Markianos-Wright
0 siblings, 0 replies; 2+ messages in thread
From: Stam Markianos-Wright @ 2022-11-22 12:43 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:0aefaf8fd036923367eb7484cab4a6a8ff01436e
commit 0aefaf8fd036923367eb7484cab4a6a8ff01436e
Author: Matthew Malcomson <matthew.malcomson@arm.com>
Date: Fri Nov 18 16:10:21 2022 +0000
Handle MEM_REF[&<something>] in gimple
Before this change, such a construct was not treated as taking the
address of the `<something>` by `walk_stmt_load_store_addr_ops` (rather
it was treated as loading or storing to `<something>` depending on how
it was used). This seems to have been working just fine since every
place which used this walk_stmt_load_store_addr_ops function either used
all callbacks (with the load and store callbacks catching these
MEM_REF[&<something>] operations), or explicitly wanted to to rewrite
MEM_REF[&<something>] constructs at the same time (i.e. whenever
`execute_update_addresses_taken` was used).
Since we disallow VIEW_CONVERT_EXPR expressions creating a capability
out of a non-capability, the rewriting that is attempted in
`execute_update_addresses_taken` is not possible.
------
First we revisit whether the state of how this operation can be
represented is satisfactory. If we've disallowed using a
VIEW_CONVERT_EXPR from non-capability to capability then the only valid
construct left for such a "treat this non-capability as a capability" is
MEM_REF[&<something>]. Until now tree-ssa.c was noticing these
constructs, marking the <something> as *not* addressed in the current
function, and attempting to rewriting the construct into a
VIEW_CONVERT_EXPR.
Our options seem to be:
- Allow VIEW_CONVERT_EXPR.
Con: Starts to allow the things that were bad before (treating
a capability as non-capability before converting to capability
later). Though the specific instances we had hit have been
fixed.
Pro: Is an existing construct which would be handled by the majority
of the code.
Pro: Does not require an unnecessary memory access and stack slot.
- We could add a new tree code or internal function to represent this.
Con: New construct, would not be seen by the majority of the code.
Con: Quite a lot of extra stuff to handle a rare case (have not hit
this until now).
Pro: Gives us a representation that does not require a memory access
and sometimes a stack slot.
- Use the existing mechanism `MEM_REF[&<something>]`.
Con: Does not allow optimisation away of a stack slot or memory
access.
Pro: Construct is already handled in the code.
Based on the frequency of these constructs (depends on the size of
things, but we hadn't hit this problem with our testsuite on purecap)
compared to the number of places that having VIEW_CONVERT_EXPR was
originally causing problems I'd lean towards using MEM_REF[&<something>]
and taking the performance hit when that construct comes up.
In general users should not be treating an __int128 as a capability, so
I'd guess this is rare.
For now I don't think that using the second option (of a new TREE code
or internal function) is sensible, though I'd be open to using that
choice later on.
------
If we're sticking with our existing representation choices, we need to
adjust something around execute_update_addresses_taken which attempts to
rewrite such constructs invalidly.
The relevant tree-ssa.c code goes through the following relevent steps:
- Iterate over all ADDR_EXPR uses in the function, recording which
decls have their address taken (*ignoring* those which have their
address taken as part of a MEM_REF[&<something>] construct).
- Remove TREE_ADDRESSABLE on local variables which do not have their
address taken. For those which also is_gimple_reg mark them as
suitable for renaming.
- Attempt to rewrite MEM_REF[&<something>] into VIEW_CONVERT_EXPR
expressions when the `something` is marked as suitable for renaming
(because it does not have its address taken).
I.e. as it stands the code believes that `MEM_REF[&<something>]` does
not mean that the `something` must be addressable, and it seems that
this is believed since the construct could be rewritten to not take the
address of that `something`. If we can not rewrite the construct, then
we must now mark `something` as addressable.
This change does so by changing `walk_stmt_load_store_addr_ops`. That
function is a pretty general function that walks over all loads, stores
and addr_expr's calling callbacks for each thing. It is used in a few
places across the compiler.
As it happens this change in when the ADDR_EXPR callback is called fits
in just fine with all existing uses.
This is mainly because all existing uses apart from the motivating case
use callbacks for loads and stores which do much the same as the
callback for addr_expr's. These callbacks are triggered on any MEM_REF
statement (whether left or right) and use `get_base_address`.
`get_base_address` looks through MEM_REF[&<something>] to get the
<something>, and act on that base declaration in the way that they would
act on it if passed via the address callback.
Hence the only functional change this patch triggers from now calling
the visit_addr callback on MEM_REF[&<something>] is around the
gimple_ior_addresses_taken -> execute_update_addresses_taken codepath,
and that's precisely the motivating case.
Making this change means that gimple_ior_addresses_taken will treat the
`<something>` declaration as having its address taken, and that means
that execute_update_addresses_taken will not remove the TREE_ADDRESSABLE
flag on that decl. Hence it will not attempt to rewrite the memory ref
base and will not trigger the ICE on attempting to create a
VIEW_CONVERT_EXPR from non-capability to capability type.
-----
N.b. there is an interesting tangential piece of information around
`MEM_REF[&<something>]` where the `<something>` is of type which does
not satisfy `is_gimple_reg_type`.
In this case execute_update_addresses_taken again marks the decl as not
TREE_ADDRESSABLE and can not rewrite the construct. On first blush this
seems problematic, since the IR includes us taking the address of
this decl even though the decl is marked as not having its address
taken.
In practice it seems that when this happens, the expand pass recognizes
these objects do not have to be stored in memory and expands them
differently (maybe even putting them onto the stack depending on their
size). See the `mem_ref_refers_to_non_mem_p` clause in the MEM_REF case
of `expand_expr_real_1`. This works partly due to variables which do
not satisfy `is_gimple_reg` are marked as TREE_USED in
`expand_used_vars`.
Whether there are other problems associated with such decls being marked
as not addressable even though there is a statement accessing their
address I'm not sure, but such problems would be pre-existing anyway.
The reason that capability_type_p variables are different here is that
they are of a type satisfying `is_gimple_reg_type`, and are hence
treated differently in the expand pass. Since they are of gimple_reg
type they are not necessarily given some RTL early on, and hence are not
recognized as being a MEM_REF referring to a non-mem location.
Diff:
---
gcc/gimple-walk.c | 18 ++++++++++++++++++
.../gcc.target/aarch64/morello/memcpy-inline-6.c | 19 +++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
index a4b42151b42..c714599478c 100644
--- a/gcc/gimple-walk.c
+++ b/gcc/gimple-walk.c
@@ -770,6 +770,24 @@ walk_stmt_load_store_addr_ops (gimple *stmt, void *data,
TREE_OPERAND (OBJ_TYPE_REF_OBJECT (val),
0), arg, data);
}
+ /* MEM_REF [&<something>] usually does not count as the address of
+ <something> being taken. This is logically the same as a
+ VIEW_CONVERT_EXPR.
+ If the MEM_REF is of capability type, and the <something> is not
+ then we have disallowed a VIEW_CONVERT_EXPR of the relevant type
+ since most such VIEW_CONVERT_EXPR's occur due to the compiler
+ attempting to do broken things. We have no representation for
+ "treat the bits of this decl as a capability" except this
+ representation through memory. Hence such a construct requires
+ the original decl to be in memory, and hence we need to pass the
+ value of this ADDR_EXPR to our callback. */
+ else if (TREE_CODE (rhs) == MEM_REF
+ && capability_type_p (TREE_TYPE (rhs))
+ && ADDR_EXPR_P (TREE_OPERAND (rhs, 0))
+ && !capability_type_p (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (rhs, 0), 0))))
+ ret |= visit_addr (stmt,
+ TREE_OPERAND (TREE_OPERAND (rhs, 0), 0),
+ arg, data);
lhs = gimple_assign_lhs (stmt);
if (TREE_CODE (lhs) == TARGET_MEM_REF
&& ADDR_EXPR_P (TMR_BASE (lhs)))
diff --git a/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c b/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c
new file mode 100644
index 00000000000..b0e44d8e652
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* This is just a testcase that shouldn't ICE.
+ The two functions below used to ICE before the patch this testcase is added
+ with. f would ICE on purecap and f2 would ICE for fakecap. */
+#ifdef __CHERI__
+unsigned __intcap c;
+#else
+unsigned __int128 c;
+#endif
+void f(void) {
+ __int128 t;
+ __builtin_memcpy (&c, &t, sizeof (t));
+}
+
+char *p;
+void f2(void) {
+ long t;
+ __builtin_memcpy (p, &t, sizeof (long));
+}
^ permalink raw reply [flat|nested] 2+ messages in thread
* [gcc(refs/vendors/ARM/heads/morello)] Handle MEM_REF[&<something>] in gimple
@ 2022-11-18 16:11 Matthew Malcomson
0 siblings, 0 replies; 2+ messages in thread
From: Matthew Malcomson @ 2022-11-18 16:11 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:0aefaf8fd036923367eb7484cab4a6a8ff01436e
commit 0aefaf8fd036923367eb7484cab4a6a8ff01436e
Author: Matthew Malcomson <matthew.malcomson@arm.com>
Date: Fri Nov 18 16:10:21 2022 +0000
Handle MEM_REF[&<something>] in gimple
Before this change, such a construct was not treated as taking the
address of the `<something>` by `walk_stmt_load_store_addr_ops` (rather
it was treated as loading or storing to `<something>` depending on how
it was used). This seems to have been working just fine since every
place which used this walk_stmt_load_store_addr_ops function either used
all callbacks (with the load and store callbacks catching these
MEM_REF[&<something>] operations), or explicitly wanted to to rewrite
MEM_REF[&<something>] constructs at the same time (i.e. whenever
`execute_update_addresses_taken` was used).
Since we disallow VIEW_CONVERT_EXPR expressions creating a capability
out of a non-capability, the rewriting that is attempted in
`execute_update_addresses_taken` is not possible.
------
First we revisit whether the state of how this operation can be
represented is satisfactory. If we've disallowed using a
VIEW_CONVERT_EXPR from non-capability to capability then the only valid
construct left for such a "treat this non-capability as a capability" is
MEM_REF[&<something>]. Until now tree-ssa.c was noticing these
constructs, marking the <something> as *not* addressed in the current
function, and attempting to rewriting the construct into a
VIEW_CONVERT_EXPR.
Our options seem to be:
- Allow VIEW_CONVERT_EXPR.
Con: Starts to allow the things that were bad before (treating
a capability as non-capability before converting to capability
later). Though the specific instances we had hit have been
fixed.
Pro: Is an existing construct which would be handled by the majority
of the code.
Pro: Does not require an unnecessary memory access and stack slot.
- We could add a new tree code or internal function to represent this.
Con: New construct, would not be seen by the majority of the code.
Con: Quite a lot of extra stuff to handle a rare case (have not hit
this until now).
Pro: Gives us a representation that does not require a memory access
and sometimes a stack slot.
- Use the existing mechanism `MEM_REF[&<something>]`.
Con: Does not allow optimisation away of a stack slot or memory
access.
Pro: Construct is already handled in the code.
Based on the frequency of these constructs (depends on the size of
things, but we hadn't hit this problem with our testsuite on purecap)
compared to the number of places that having VIEW_CONVERT_EXPR was
originally causing problems I'd lean towards using MEM_REF[&<something>]
and taking the performance hit when that construct comes up.
In general users should not be treating an __int128 as a capability, so
I'd guess this is rare.
For now I don't think that using the second option (of a new TREE code
or internal function) is sensible, though I'd be open to using that
choice later on.
------
If we're sticking with our existing representation choices, we need to
adjust something around execute_update_addresses_taken which attempts to
rewrite such constructs invalidly.
The relevant tree-ssa.c code goes through the following relevent steps:
- Iterate over all ADDR_EXPR uses in the function, recording which
decls have their address taken (*ignoring* those which have their
address taken as part of a MEM_REF[&<something>] construct).
- Remove TREE_ADDRESSABLE on local variables which do not have their
address taken. For those which also is_gimple_reg mark them as
suitable for renaming.
- Attempt to rewrite MEM_REF[&<something>] into VIEW_CONVERT_EXPR
expressions when the `something` is marked as suitable for renaming
(because it does not have its address taken).
I.e. as it stands the code believes that `MEM_REF[&<something>]` does
not mean that the `something` must be addressable, and it seems that
this is believed since the construct could be rewritten to not take the
address of that `something`. If we can not rewrite the construct, then
we must now mark `something` as addressable.
This change does so by changing `walk_stmt_load_store_addr_ops`. That
function is a pretty general function that walks over all loads, stores
and addr_expr's calling callbacks for each thing. It is used in a few
places across the compiler.
As it happens this change in when the ADDR_EXPR callback is called fits
in just fine with all existing uses.
This is mainly because all existing uses apart from the motivating case
use callbacks for loads and stores which do much the same as the
callback for addr_expr's. These callbacks are triggered on any MEM_REF
statement (whether left or right) and use `get_base_address`.
`get_base_address` looks through MEM_REF[&<something>] to get the
<something>, and act on that base declaration in the way that they would
act on it if passed via the address callback.
Hence the only functional change this patch triggers from now calling
the visit_addr callback on MEM_REF[&<something>] is around the
gimple_ior_addresses_taken -> execute_update_addresses_taken codepath,
and that's precisely the motivating case.
Making this change means that gimple_ior_addresses_taken will treat the
`<something>` declaration as having its address taken, and that means
that execute_update_addresses_taken will not remove the TREE_ADDRESSABLE
flag on that decl. Hence it will not attempt to rewrite the memory ref
base and will not trigger the ICE on attempting to create a
VIEW_CONVERT_EXPR from non-capability to capability type.
-----
N.b. there is an interesting tangential piece of information around
`MEM_REF[&<something>]` where the `<something>` is of type which does
not satisfy `is_gimple_reg_type`.
In this case execute_update_addresses_taken again marks the decl as not
TREE_ADDRESSABLE and can not rewrite the construct. On first blush this
seems problematic, since the IR includes us taking the address of
this decl even though the decl is marked as not having its address
taken.
In practice it seems that when this happens, the expand pass recognizes
these objects do not have to be stored in memory and expands them
differently (maybe even putting them onto the stack depending on their
size). See the `mem_ref_refers_to_non_mem_p` clause in the MEM_REF case
of `expand_expr_real_1`. This works partly due to variables which do
not satisfy `is_gimple_reg` are marked as TREE_USED in
`expand_used_vars`.
Whether there are other problems associated with such decls being marked
as not addressable even though there is a statement accessing their
address I'm not sure, but such problems would be pre-existing anyway.
The reason that capability_type_p variables are different here is that
they are of a type satisfying `is_gimple_reg_type`, and are hence
treated differently in the expand pass. Since they are of gimple_reg
type they are not necessarily given some RTL early on, and hence are not
recognized as being a MEM_REF referring to a non-mem location.
Diff:
---
gcc/gimple-walk.c | 18 ++++++++++++++++++
.../gcc.target/aarch64/morello/memcpy-inline-6.c | 19 +++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
index a4b42151b42..c714599478c 100644
--- a/gcc/gimple-walk.c
+++ b/gcc/gimple-walk.c
@@ -770,6 +770,24 @@ walk_stmt_load_store_addr_ops (gimple *stmt, void *data,
TREE_OPERAND (OBJ_TYPE_REF_OBJECT (val),
0), arg, data);
}
+ /* MEM_REF [&<something>] usually does not count as the address of
+ <something> being taken. This is logically the same as a
+ VIEW_CONVERT_EXPR.
+ If the MEM_REF is of capability type, and the <something> is not
+ then we have disallowed a VIEW_CONVERT_EXPR of the relevant type
+ since most such VIEW_CONVERT_EXPR's occur due to the compiler
+ attempting to do broken things. We have no representation for
+ "treat the bits of this decl as a capability" except this
+ representation through memory. Hence such a construct requires
+ the original decl to be in memory, and hence we need to pass the
+ value of this ADDR_EXPR to our callback. */
+ else if (TREE_CODE (rhs) == MEM_REF
+ && capability_type_p (TREE_TYPE (rhs))
+ && ADDR_EXPR_P (TREE_OPERAND (rhs, 0))
+ && !capability_type_p (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (rhs, 0), 0))))
+ ret |= visit_addr (stmt,
+ TREE_OPERAND (TREE_OPERAND (rhs, 0), 0),
+ arg, data);
lhs = gimple_assign_lhs (stmt);
if (TREE_CODE (lhs) == TARGET_MEM_REF
&& ADDR_EXPR_P (TMR_BASE (lhs)))
diff --git a/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c b/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c
new file mode 100644
index 00000000000..b0e44d8e652
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/morello/memcpy-inline-6.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* This is just a testcase that shouldn't ICE.
+ The two functions below used to ICE before the patch this testcase is added
+ with. f would ICE on purecap and f2 would ICE for fakecap. */
+#ifdef __CHERI__
+unsigned __intcap c;
+#else
+unsigned __int128 c;
+#endif
+void f(void) {
+ __int128 t;
+ __builtin_memcpy (&c, &t, sizeof (t));
+}
+
+char *p;
+void f2(void) {
+ long t;
+ __builtin_memcpy (p, &t, sizeof (long));
+}
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-22 12:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-22 12:43 [gcc(refs/vendors/ARM/heads/morello)] Handle MEM_REF[&<something>] in gimple Stam Markianos-Wright
-- strict thread matches above, loose matches on Subject: below --
2022-11-18 16:11 Matthew Malcomson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).