* [PR 70646] Store size to inlining predicate conditions
@ 2016-05-11 15:45 Martin Jambor
[not found] ` <8402b16805536e57e9c06ee1fe1992c0@suse.de>
0 siblings, 1 reply; 2+ messages in thread
From: Martin Jambor @ 2016-05-11 15:45 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka
Hi,
PR 70646 takes place because inlining predicate evaluation does not
check for memory access size when evaluating IS_NOT_CONSTANT
conditions. This means that a smaller stored constant is evaluated as
disproving an IS_NOT_CONSTANT condition even though that was meant for
a larger memory read. Inlining in turn made the conclusion that a
branch can never be taken and turned all calls there into
__builtin_unreachables, eliminating the branch in practice. However,
intraprocedural folding did not evaluate the __builtin_constant as
true and decided to execute the removed branch.
The fix below stores the intended size into each condition and then
compares it with the size of the actual constant that is propagated
from a caller, with the exception of CHANGED conditions, which are
evaluated to false when error_mark_node is propagated instead of a
constant and which has no size. But that is OK because CHANGED is
used only for inlining heuristics and in principle cannot be used to
optimize out anything completely.
As noted in bugzilla, I use singed HOST_WIDE_INT for the size so that
it is consistent with what get_ref_base_and_extent uses to return
size, which is what we use to get it for aggregate values.
Bootstrapped, lto-bootstrapped and tested on x86_64-linux without any
issues. OK for trunk and all active release branches? (4.9 still is
active, right?)
Thanks,
Martin
2016-04-20 Martin Jambor <mjambor@suse.cz>
PR ipa/70646
* ipa-inline.h (condition): New field size.
* ipa-inline-analysis.c (add_condition): New parameter SIZE, use it
for comaprison and store it into the new condition.
(evaluate_conditions_for_known_args): Use condition size to check
access sizes for all but CHANGED conditions.
(unmodified_parm_1): New parameter size_p, store access size into it.
(unmodified_parm): Likewise.
(unmodified_parm_or_parm_agg_item): Likewise.
(eliminated_by_inlining_prob): Pass NULL to unmodified_parm as size_p.
(set_cond_stmt_execution_predicate): Extract access sizes and store
them to conditions.
(set_switch_stmt_execution_predicate): Likewise.
(will_be_nonconstant_expr_predicate): Likewise.
(will_be_nonconstant_predicate): Likewise.
(inline_read_section): Stream condition size.
(inline_write_summary): Likewise.
testsuite/
*gcc.dg/ipa/pr70646.c: New test.
---
gcc/ipa-inline-analysis.c | 132 +++++++++++++++++++++++--------------
gcc/ipa-inline.h | 2 +
gcc/testsuite/gcc.dg/ipa/pr70646.c | 40 +++++++++++
3 files changed, 123 insertions(+), 51 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr70646.c
diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index 17b21d1..68824f7 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -216,13 +216,14 @@ struct agg_position_info
bool by_ref;
};
-/* Add condition to condition list CONDS. AGGPOS describes whether the used
- oprand is loaded from an aggregate and where in the aggregate it is. It can
- be NULL, which means this not a load from an aggregate. */
+/* Add condition to condition list SUMMARY. OPERAND_NUM, SIZE, CODE and VAL
+ correspond to fields of condition structure. AGGPOS describes whether the
+ used operand is loaded from an aggregate and where in the aggregate it is.
+ It can be NULL, which means this not a load from an aggregate. */
static struct predicate
add_condition (struct inline_summary *summary, int operand_num,
- struct agg_position_info *aggpos,
+ HOST_WIDE_INT size, struct agg_position_info *aggpos,
enum tree_code code, tree val)
{
int i;
@@ -248,6 +249,7 @@ add_condition (struct inline_summary *summary, int operand_num,
for (i = 0; vec_safe_iterate (summary->conds, i, &c); i++)
{
if (c->operand_num == operand_num
+ && c->size == size
&& c->code == code
&& c->val == val
&& c->agg_contents == agg_contents
@@ -264,6 +266,7 @@ add_condition (struct inline_summary *summary, int operand_num,
new_cond.agg_contents = agg_contents;
new_cond.by_ref = by_ref;
new_cond.offset = offset;
+ new_cond.size = size;
vec_safe_push (summary->conds, new_cond);
return single_cond_predicate (i + predicate_first_dynamic_condition);
}
@@ -867,21 +870,25 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
clause |= 1 << (i + predicate_first_dynamic_condition);
continue;
}
- if (c->code == IS_NOT_CONSTANT || c->code == CHANGED)
+ if (c->code == CHANGED)
continue;
- if (operand_equal_p (TYPE_SIZE (TREE_TYPE (c->val)),
- TYPE_SIZE (TREE_TYPE (val)), 0))
+ if (tree_to_shwi (TYPE_SIZE (TREE_TYPE (val))) != c->size)
{
- val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c->val), val);
+ clause |= 1 << (i + predicate_first_dynamic_condition);
+ continue;
+ }
+ if (c->code == IS_NOT_CONSTANT)
+ continue;
- res = val
- ? fold_binary_to_constant (c->code, boolean_type_node, val, c->val)
- : NULL;
+ val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c->val), val);
+ res = val
+ ? fold_binary_to_constant (c->code, boolean_type_node, val, c->val)
+ : NULL;
+
+ if (res && integer_zerop (res))
+ continue;
- if (res && integer_zerop (res))
- continue;
- }
clause |= 1 << (i + predicate_first_dynamic_condition);
}
return clause;
@@ -1522,16 +1529,21 @@ mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef ATTRIBUTE_UNUSED,
}
/* If OP refers to value of function parameter, return the corresponding
- parameter. */
+ parameter. If non-NULL, the size of the memory load (or the SSA_NAME of the
+ PARM_DECL) will be stored to *SIZE_P in that case too. */
static tree
-unmodified_parm_1 (gimple *stmt, tree op)
+unmodified_parm_1 (gimple *stmt, tree op, HOST_WIDE_INT *size_p)
{
/* SSA_NAME referring to parm default def? */
if (TREE_CODE (op) == SSA_NAME
&& SSA_NAME_IS_DEFAULT_DEF (op)
&& TREE_CODE (SSA_NAME_VAR (op)) == PARM_DECL)
- return SSA_NAME_VAR (op);
+ {
+ if (size_p)
+ *size_p = tree_to_shwi (TYPE_SIZE (TREE_TYPE (op)));
+ return SSA_NAME_VAR (op);
+ }
/* Non-SSA parm reference? */
if (TREE_CODE (op) == PARM_DECL)
{
@@ -1542,18 +1554,24 @@ unmodified_parm_1 (gimple *stmt, tree op)
walk_aliased_vdefs (&refd, gimple_vuse (stmt), mark_modified, &modified,
NULL);
if (!modified)
- return op;
+ {
+ if (size_p)
+ *size_p = tree_to_shwi (TYPE_SIZE (TREE_TYPE (op)));
+ return op;
+ }
}
return NULL_TREE;
}
/* If OP refers to value of function parameter, return the corresponding
- parameter. Also traverse chains of SSA register assignments. */
+ parameter. Also traverse chains of SSA register assignments. If non-NULL,
+ the size of the memory load (or the SSA_NAME of the PARM_DECL) will be
+ stored to *SIZE_P in that case too. */
static tree
-unmodified_parm (gimple *stmt, tree op)
+unmodified_parm (gimple *stmt, tree op, HOST_WIDE_INT *size_p)
{
- tree res = unmodified_parm_1 (stmt, op);
+ tree res = unmodified_parm_1 (stmt, op, size_p);
if (res)
return res;
@@ -1561,23 +1579,25 @@ unmodified_parm (gimple *stmt, tree op)
&& !SSA_NAME_IS_DEFAULT_DEF (op)
&& gimple_assign_single_p (SSA_NAME_DEF_STMT (op)))
return unmodified_parm (SSA_NAME_DEF_STMT (op),
- gimple_assign_rhs1 (SSA_NAME_DEF_STMT (op)));
+ gimple_assign_rhs1 (SSA_NAME_DEF_STMT (op)),
+ size_p);
return NULL_TREE;
}
/* If OP refers to a value of a function parameter or value loaded from an
aggregate passed to a parameter (either by value or reference), return TRUE
- and store the number of the parameter to *INDEX_P and information whether
- and how it has been loaded from an aggregate into *AGGPOS. INFO describes
- the function parameters, STMT is the statement in which OP is used or
- loaded. */
+ and store the number of the parameter to *INDEX_P, the access size into
+ *SIZE_P, and information whether and how it has been loaded from an
+ aggregate into *AGGPOS. INFO describes the function parameters, STMT is the
+ statement in which OP is used or loaded. */
static bool
unmodified_parm_or_parm_agg_item (struct ipa_func_body_info *fbi,
gimple *stmt, tree op, int *index_p,
+ HOST_WIDE_INT *size_p,
struct agg_position_info *aggpos)
{
- tree res = unmodified_parm_1 (stmt, op);
+ tree res = unmodified_parm_1 (stmt, op, size_p);
gcc_checking_assert (aggpos);
if (res)
@@ -1598,14 +1618,14 @@ unmodified_parm_or_parm_agg_item (struct ipa_func_body_info *fbi,
stmt = SSA_NAME_DEF_STMT (op);
op = gimple_assign_rhs1 (stmt);
if (!REFERENCE_CLASS_P (op))
- return unmodified_parm_or_parm_agg_item (fbi, stmt, op, index_p,
+ return unmodified_parm_or_parm_agg_item (fbi, stmt, op, index_p, size_p,
aggpos);
}
aggpos->agg_contents = true;
return ipa_load_from_parm_agg (fbi, fbi->info->descriptors,
stmt, op, index_p, &aggpos->offset,
- NULL, &aggpos->by_ref);
+ size_p, &aggpos->by_ref);
}
/* See if statement might disappear after inlining.
@@ -1656,7 +1676,7 @@ eliminated_by_inlining_prob (gimple *stmt)
inner_lhs = lhs;
/* Reads of parameter are expected to be free. */
- if (unmodified_parm (stmt, inner_rhs))
+ if (unmodified_parm (stmt, inner_rhs, NULL))
rhs_free = true;
/* Match expressions of form &this->field. Those will most likely
combine with something upstream after inlining. */
@@ -1666,7 +1686,7 @@ eliminated_by_inlining_prob (gimple *stmt)
if (TREE_CODE (op) == PARM_DECL)
rhs_free = true;
else if (TREE_CODE (op) == MEM_REF
- && unmodified_parm (stmt, TREE_OPERAND (op, 0)))
+ && unmodified_parm (stmt, TREE_OPERAND (op, 0), NULL))
rhs_free = true;
}
@@ -1679,7 +1699,7 @@ eliminated_by_inlining_prob (gimple *stmt)
/* Reads of parameters passed by reference
expected to be free (i.e. optimized out after inlining). */
if (TREE_CODE (inner_rhs) == MEM_REF
- && unmodified_parm (stmt, TREE_OPERAND (inner_rhs, 0)))
+ && unmodified_parm (stmt, TREE_OPERAND (inner_rhs, 0), NULL))
rhs_free = true;
/* Copying parameter passed by reference into gimple register is
@@ -1720,7 +1740,7 @@ eliminated_by_inlining_prob (gimple *stmt)
if (TREE_CODE (inner_lhs) == PARM_DECL
|| TREE_CODE (inner_lhs) == RESULT_DECL
|| (TREE_CODE (inner_lhs) == MEM_REF
- && (unmodified_parm (stmt, TREE_OPERAND (inner_lhs, 0))
+ && (unmodified_parm (stmt, TREE_OPERAND (inner_lhs, 0), NULL)
|| (TREE_CODE (TREE_OPERAND (inner_lhs, 0)) == SSA_NAME
&& SSA_NAME_VAR (TREE_OPERAND (inner_lhs, 0))
&& TREE_CODE (SSA_NAME_VAR (TREE_OPERAND
@@ -1751,6 +1771,7 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi,
gimple *last;
tree op;
int index;
+ HOST_WIDE_INT size;
struct agg_position_info aggpos;
enum tree_code code, inverted_code;
edge e;
@@ -1767,7 +1788,7 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi,
/* TODO: handle conditionals like
var = op0 < 4;
if (var != 0). */
- if (unmodified_parm_or_parm_agg_item (fbi, last, op, &index, &aggpos))
+ if (unmodified_parm_or_parm_agg_item (fbi, last, op, &index, &size, &aggpos))
{
code = gimple_cond_code (last);
inverted_code = invert_tree_comparison (code, HONOR_NANS (op));
@@ -1781,9 +1802,10 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi,
unordered one. Be sure it is not confused with NON_CONSTANT. */
if (this_code != ERROR_MARK)
{
- struct predicate p = add_condition
- (summary, index, &aggpos, this_code,
- unshare_expr_without_location (gimple_cond_rhs (last)));
+ struct predicate p
+ = add_condition (summary, index, size, &aggpos, this_code,
+ unshare_expr_without_location
+ (gimple_cond_rhs (last)));
e->aux = edge_predicate_pool.allocate ();
*(struct predicate *) e->aux = p;
}
@@ -1810,11 +1832,12 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi,
|| gimple_call_num_args (set_stmt) != 1)
return;
op2 = gimple_call_arg (set_stmt, 0);
- if (!unmodified_parm_or_parm_agg_item (fbi, set_stmt, op2, &index, &aggpos))
+ if (!unmodified_parm_or_parm_agg_item (fbi, set_stmt, op2, &index, &size,
+ &aggpos))
return;
FOR_EACH_EDGE (e, ei, bb->succs) if (e->flags & EDGE_FALSE_VALUE)
{
- struct predicate p = add_condition (summary, index, &aggpos,
+ struct predicate p = add_condition (summary, index, size, &aggpos,
IS_NOT_CONSTANT, NULL_TREE);
e->aux = edge_predicate_pool.allocate ();
*(struct predicate *) e->aux = p;
@@ -1833,6 +1856,7 @@ set_switch_stmt_execution_predicate (struct ipa_func_body_info *fbi,
gimple *lastg;
tree op;
int index;
+ HOST_WIDE_INT size;
struct agg_position_info aggpos;
edge e;
edge_iterator ei;
@@ -1844,7 +1868,7 @@ set_switch_stmt_execution_predicate (struct ipa_func_body_info *fbi,
return;
gswitch *last = as_a <gswitch *> (lastg);
op = gimple_switch_index (last);
- if (!unmodified_parm_or_parm_agg_item (fbi, last, op, &index, &aggpos))
+ if (!unmodified_parm_or_parm_agg_item (fbi, last, op, &index, &size, &aggpos))
return;
FOR_EACH_EDGE (e, ei, bb->succs)
@@ -1869,14 +1893,14 @@ set_switch_stmt_execution_predicate (struct ipa_func_body_info *fbi,
if (!min && !max)
p = true_predicate ();
else if (!max)
- p = add_condition (summary, index, &aggpos, EQ_EXPR,
+ p = add_condition (summary, index, size, &aggpos, EQ_EXPR,
unshare_expr_without_location (min));
else
{
struct predicate p1, p2;
- p1 = add_condition (summary, index, &aggpos, GE_EXPR,
+ p1 = add_condition (summary, index, size, &aggpos, GE_EXPR,
unshare_expr_without_location (min));
- p2 = add_condition (summary, index, &aggpos, LE_EXPR,
+ p2 = add_condition (summary, index, size, &aggpos, LE_EXPR,
unshare_expr_without_location (max));
p = and_predicates (summary->conds, &p1, &p2);
}
@@ -1977,13 +2001,14 @@ will_be_nonconstant_expr_predicate (struct ipa_node_params *info,
{
tree parm;
int index;
+ HOST_WIDE_INT size;
while (UNARY_CLASS_P (expr))
expr = TREE_OPERAND (expr, 0);
- parm = unmodified_parm (NULL, expr);
+ parm = unmodified_parm (NULL, expr, &size);
if (parm && (index = ipa_get_param_decl_index (info, parm)) >= 0)
- return add_condition (summary, index, NULL, CHANGED, NULL_TREE);
+ return add_condition (summary, index, size, NULL, CHANGED, NULL_TREE);
if (is_gimple_min_invariant (expr))
return false_predicate ();
if (TREE_CODE (expr) == SSA_NAME)
@@ -2044,6 +2069,7 @@ will_be_nonconstant_predicate (struct ipa_func_body_info *fbi,
struct predicate op_non_const;
bool is_load;
int base_index;
+ HOST_WIDE_INT size;
struct agg_position_info aggpos;
/* What statments might be optimized away
@@ -2067,7 +2093,7 @@ will_be_nonconstant_predicate (struct ipa_func_body_info *fbi,
tree op;
gcc_assert (gimple_assign_single_p (stmt));
op = gimple_assign_rhs1 (stmt);
- if (!unmodified_parm_or_parm_agg_item (fbi, stmt, op, &base_index,
+ if (!unmodified_parm_or_parm_agg_item (fbi, stmt, op, &base_index, &size,
&aggpos))
return p;
}
@@ -2078,7 +2104,7 @@ will_be_nonconstant_predicate (struct ipa_func_body_info *fbi,
adding conditionals. */
FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
{
- tree parm = unmodified_parm (stmt, use);
+ tree parm = unmodified_parm (stmt, use, NULL);
/* For arguments we can build a condition. */
if (parm && ipa_get_param_decl_index (fbi->info, parm) >= 0)
continue;
@@ -2093,18 +2119,19 @@ will_be_nonconstant_predicate (struct ipa_func_body_info *fbi,
if (is_load)
op_non_const =
- add_condition (summary, base_index, &aggpos, CHANGED, NULL);
+ add_condition (summary, base_index, size, &aggpos, CHANGED, NULL);
else
op_non_const = false_predicate ();
FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
{
- tree parm = unmodified_parm (stmt, use);
+ HOST_WIDE_INT size;
+ tree parm = unmodified_parm (stmt, use, &size);
int index;
if (parm && (index = ipa_get_param_decl_index (fbi->info, parm)) >= 0)
{
if (index != base_index)
- p = add_condition (summary, index, NULL, CHANGED, NULL_TREE);
+ p = add_condition (summary, index, size, NULL, CHANGED, NULL_TREE);
else
continue;
}
@@ -3407,7 +3434,8 @@ remap_predicate (struct inline_summary *info,
ap.by_ref = c->by_ref;
cond_predicate = add_condition (info,
operand_map[c->operand_num],
- &ap, c->code, c->val);
+ c->size, &ap, c->code,
+ c->val);
}
}
/* Fixed conditions remains same, construct single
@@ -4261,6 +4289,7 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data,
{
struct condition c;
c.operand_num = streamer_read_uhwi (&ib);
+ c.size = streamer_read_uhwi (&ib);
c.code = (enum tree_code) streamer_read_uhwi (&ib);
c.val = stream_read_tree (&ib, data_in);
bp = streamer_read_bitpack (&ib);
@@ -4426,6 +4455,7 @@ inline_write_summary (void)
for (i = 0; vec_safe_iterate (info->conds, i, &c); i++)
{
streamer_write_uhwi (ob, c->operand_num);
+ streamer_write_uhwi (ob, c->size);
streamer_write_uhwi (ob, c->code);
stream_write_tree (ob, c->val, true);
bp = bitpack_create (ob->main_stream);
diff --git a/gcc/ipa-inline.h b/gcc/ipa-inline.h
index df535d0..47f8832 100644
--- a/gcc/ipa-inline.h
+++ b/gcc/ipa-inline.h
@@ -34,6 +34,8 @@ struct GTY(()) condition
/* If agg_contents is set, this is the offset from which the used data was
loaded. */
HOST_WIDE_INT offset;
+ /* Size of the access reading the data (or the PARM_DECL SSA_NAME). */
+ HOST_WIDE_INT size;
tree val;
int operand_num;
ENUM_BITFIELD(tree_code) code : 16;
diff --git a/gcc/testsuite/gcc.dg/ipa/pr70646.c b/gcc/testsuite/gcc.dg/ipa/pr70646.c
new file mode 100644
index 0000000..f85816e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr70646.c
@@ -0,0 +1,40 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#pragma GCC optimize("no-unit-at-a-time")
+
+typedef unsigned char u8;
+typedef unsigned long long u64;
+
+static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
+{
+ return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & (u64)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 56))) : __builtin_bswap64(*p));
+}
+
+static inline u64 wwn_to_u64(void *wwn)
+{
+ return __swab64p(wwn);
+}
+
+void __attribute__((noinline,noclone)) broken(u64* shost)
+{
+ u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
+ *shost = wwn_to_u64(node_name);
+}
+
+void __attribute__((noinline,noclone)) dummy(void)
+{
+ __builtin_abort();
+}
+
+int main(int argc, char* argv[])
+{
+ u64 v;
+
+ broken(&v);
+
+ if(v != (u64)-1)
+ __builtin_abort();
+
+ return 0;
+}
--
2.8.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PR 70646] Store size to inlining predicate conditions
[not found] ` <8402b16805536e57e9c06ee1fe1992c0@suse.de>
@ 2016-05-19 14:38 ` Martin Jambor
0 siblings, 0 replies; 2+ messages in thread
From: Martin Jambor @ 2016-05-19 14:38 UTC (permalink / raw)
To: GCC Patches
Hi,
On Wed, May 18, 2016 at 12:19:11PM +0200, jh wrote:
> Dne 2016-05-11 17:45, Martin Jambor napsal:
> > Hi,
> >
> >
> > 2016-04-20 Martin Jambor <mjambor@suse.cz>
> >
> > PR ipa/70646
> > * ipa-inline.h (condition): New field size.
> > * ipa-inline-analysis.c (add_condition): New parameter SIZE, use it
> > for comaprison and store it into the new condition.
> > (evaluate_conditions_for_known_args): Use condition size to check
> > access sizes for all but CHANGED conditions.
> > (unmodified_parm_1): New parameter size_p, store access size into it.
> > (unmodified_parm): Likewise.
> > (unmodified_parm_or_parm_agg_item): Likewise.
> > (eliminated_by_inlining_prob): Pass NULL to unmodified_parm as size_p.
> > (set_cond_stmt_execution_predicate): Extract access sizes and store
> > them to conditions.
> > (set_switch_stmt_execution_predicate): Likewise.
> > (will_be_nonconstant_expr_predicate): Likewise.
> > (will_be_nonconstant_predicate): Likewise.
> > (inline_read_section): Stream condition size.
> > (inline_write_summary): Likewise.
>
> This is OK for mainline and branches week later. You will need to bump up
> the LTO stream
> revision. Just one question:
Thanks, I have committed the patch to trunk and added hunks bumping
the minor LTO versions to the patches for release versions.
>
> Thanks,
> Honza
> >
> > - if (operand_equal_p (TYPE_SIZE (TREE_TYPE (c->val)),
> > - TYPE_SIZE (TREE_TYPE (val)), 0))
> > + if (tree_to_shwi (TYPE_SIZE (TREE_TYPE (val))) != c->size)
>
> Will it work for variable sized types and/or types whose size will not fit
> in SHWI? Can these happen here or they are ruled out earlier in alaysis?
>
At this point, val needs to be na ipa_invariant, which implies fixed
and small size.
The patch I posted applies as-is to the gcc-6 branch but I had to do
some manual backporting for the gcc-5 and gcc-4_9 branches. In the
latter, I had to change the prototype of ipa_load_from_parm_agg to
also return size of the load.
For the reference, the 4.9 patch is below. I have bootstrapped and
tested the different alternatives for all branches and will start
committing them now.
Thanks a lot,
Martin
2016-05-18 Martin Jambor <mjambor@suse.cz>
PR ipa/70646
* ipa-inline.h (condition): New field size.
* ipa-inline-analysis.c (add_condition): New parameter SIZE, use it
for comaprison and store it into the new condition.
(evaluate_conditions_for_known_args): Use condition size to check
access sizes for all but CHANGED conditions.
(unmodified_parm_1): New parameter size_p, store access size into it.
(unmodified_parm): Likewise.
(unmodified_parm_or_parm_agg_item): Likewise.
(eliminated_by_inlining_prob): Pass NULL to unmodified_parm as size_p.
(set_cond_stmt_execution_predicate): Extract access sizes and store
them to conditions.
(set_switch_stmt_execution_predicate): Likewise.
(will_be_nonconstant_expr_predicate): Likewise.
(will_be_nonconstant_predicate): Likewise.
(inline_read_section): Stream condition size.
(inline_write_summary): Likewise.
* lto-streamer.h (LTO_minor_version): Bump.
* ipa-prop.c (ipa_load_from_parm_agg): Added size_p parameter, pass it
to ipa_load_from_parm_agg_1.
* ipa-prop.h (ipa_load_from_parm_agg): Update declaration.
testsuite/
* gcc.dg/ipa/pr70646.c: New test.
---
gcc/ipa-inline-analysis.c | 125 ++++++++++++++++++++++---------------
gcc/ipa-inline.h | 2 +
gcc/ipa-prop.c | 4 +-
gcc/ipa-prop.h | 2 +-
gcc/lto-streamer.h | 2 +-
gcc/testsuite/gcc.dg/ipa/pr70646.c | 40 ++++++++++++
6 files changed, 122 insertions(+), 53 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/pr70646.c
diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index 9e71f43..47a3810 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -233,13 +233,14 @@ struct agg_position_info
bool by_ref;
};
-/* Add condition to condition list CONDS. AGGPOS describes whether the used
- oprand is loaded from an aggregate and where in the aggregate it is. It can
- be NULL, which means this not a load from an aggregate. */
+/* Add condition to condition list SUMMARY. OPERAND_NUM, SIZE, CODE and VAL
+ correspond to fields of condition structure. AGGPOS describes whether the
+ used operand is loaded from an aggregate and where in the aggregate it is.
+ It can be NULL, which means this not a load from an aggregate. */
static struct predicate
add_condition (struct inline_summary *summary, int operand_num,
- struct agg_position_info *aggpos,
+ HOST_WIDE_INT size, struct agg_position_info *aggpos,
enum tree_code code, tree val)
{
int i;
@@ -265,6 +266,7 @@ add_condition (struct inline_summary *summary, int operand_num,
for (i = 0; vec_safe_iterate (summary->conds, i, &c); i++)
{
if (c->operand_num == operand_num
+ && c->size == size
&& c->code == code
&& c->val == val
&& c->agg_contents == agg_contents
@@ -281,6 +283,7 @@ add_condition (struct inline_summary *summary, int operand_num,
new_cond.agg_contents = agg_contents;
new_cond.by_ref = by_ref;
new_cond.offset = offset;
+ new_cond.size = size;
vec_safe_push (summary->conds, new_cond);
return single_cond_predicate (i + predicate_first_dynamic_condition);
}
@@ -859,21 +862,25 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
clause |= 1 << (i + predicate_first_dynamic_condition);
continue;
}
- if (c->code == IS_NOT_CONSTANT || c->code == CHANGED)
+ if (c->code == CHANGED)
continue;
- if (operand_equal_p (TYPE_SIZE (TREE_TYPE (c->val)),
- TYPE_SIZE (TREE_TYPE (val)), 0))
+ if (tree_to_shwi (TYPE_SIZE (TREE_TYPE (val))) != c->size)
{
- val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c->val), val);
+ clause |= 1 << (i + predicate_first_dynamic_condition);
+ continue;
+ }
+ if (c->code == IS_NOT_CONSTANT)
+ continue;
- res = val
- ? fold_binary_to_constant (c->code, boolean_type_node, val, c->val)
- : NULL;
+ val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c->val), val);
+ res = val
+ ? fold_binary_to_constant (c->code, boolean_type_node, val, c->val)
+ : NULL;
+
+ if (res && integer_zerop (res))
+ continue;
- if (res && integer_zerop (res))
- continue;
- }
clause |= 1 << (i + predicate_first_dynamic_condition);
}
return clause;
@@ -1495,16 +1502,21 @@ mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef ATTRIBUTE_UNUSED,
}
/* If OP refers to value of function parameter, return the corresponding
- parameter. */
+ parameter. If non-NULL, the size of the memory load (or the SSA_NAME of the
+ PARM_DECL) will be stored to *SIZE_P in that case too. */
static tree
-unmodified_parm_1 (gimple stmt, tree op)
+unmodified_parm_1 (gimple stmt, tree op, HOST_WIDE_INT *size_p)
{
/* SSA_NAME referring to parm default def? */
if (TREE_CODE (op) == SSA_NAME
&& SSA_NAME_IS_DEFAULT_DEF (op)
&& TREE_CODE (SSA_NAME_VAR (op)) == PARM_DECL)
- return SSA_NAME_VAR (op);
+ {
+ if (size_p)
+ *size_p = tree_to_shwi (TYPE_SIZE (TREE_TYPE (op)));
+ return SSA_NAME_VAR (op);
+ }
/* Non-SSA parm reference? */
if (TREE_CODE (op) == PARM_DECL)
{
@@ -1515,18 +1527,24 @@ unmodified_parm_1 (gimple stmt, tree op)
walk_aliased_vdefs (&refd, gimple_vuse (stmt), mark_modified, &modified,
NULL);
if (!modified)
- return op;
+ {
+ if (size_p)
+ *size_p = tree_to_shwi (TYPE_SIZE (TREE_TYPE (op)));
+ return op;
+ }
}
return NULL_TREE;
}
/* If OP refers to value of function parameter, return the corresponding
- parameter. Also traverse chains of SSA register assignments. */
+ parameter. Also traverse chains of SSA register assignments. If non-NULL,
+ the size of the memory load (or the SSA_NAME of the PARM_DECL) will be
+ stored to *SIZE_P in that case too. */
static tree
-unmodified_parm (gimple stmt, tree op)
+unmodified_parm (gimple stmt, tree op, HOST_WIDE_INT *size_p)
{
- tree res = unmodified_parm_1 (stmt, op);
+ tree res = unmodified_parm_1 (stmt, op, size_p);
if (res)
return res;
@@ -1534,23 +1552,25 @@ unmodified_parm (gimple stmt, tree op)
&& !SSA_NAME_IS_DEFAULT_DEF (op)
&& gimple_assign_single_p (SSA_NAME_DEF_STMT (op)))
return unmodified_parm (SSA_NAME_DEF_STMT (op),
- gimple_assign_rhs1 (SSA_NAME_DEF_STMT (op)));
+ gimple_assign_rhs1 (SSA_NAME_DEF_STMT (op)),
+ size_p);
return NULL_TREE;
}
/* If OP refers to a value of a function parameter or value loaded from an
aggregate passed to a parameter (either by value or reference), return TRUE
- and store the number of the parameter to *INDEX_P and information whether
- and how it has been loaded from an aggregate into *AGGPOS. INFO describes
- the function parameters, STMT is the statement in which OP is used or
- loaded. */
+ and store the number of the parameter to *INDEX_P, the access size into
+ *SIZE_P, and information whether and how it has been loaded from an
+ aggregate into *AGGPOS. INFO describes the function parameters, STMT is the
+ statement in which OP is used or loaded. */
static bool
unmodified_parm_or_parm_agg_item (struct ipa_node_params *info,
gimple stmt, tree op, int *index_p,
+ HOST_WIDE_INT *size_p,
struct agg_position_info *aggpos)
{
- tree res = unmodified_parm_1 (stmt, op);
+ tree res = unmodified_parm_1 (stmt, op, size_p);
gcc_checking_assert (aggpos);
if (res)
@@ -1572,12 +1592,12 @@ unmodified_parm_or_parm_agg_item (struct ipa_node_params *info,
op = gimple_assign_rhs1 (stmt);
if (!REFERENCE_CLASS_P (op))
return unmodified_parm_or_parm_agg_item (info, stmt, op, index_p,
- aggpos);
+ size_p, aggpos);
}
aggpos->agg_contents = true;
return ipa_load_from_parm_agg (info, stmt, op, index_p, &aggpos->offset,
- &aggpos->by_ref);
+ size_p, &aggpos->by_ref);
}
/* See if statement might disappear after inlining.
@@ -1629,7 +1649,7 @@ eliminated_by_inlining_prob (gimple stmt)
inner_lhs = lhs;
/* Reads of parameter are expected to be free. */
- if (unmodified_parm (stmt, inner_rhs))
+ if (unmodified_parm (stmt, inner_rhs, NULL))
rhs_free = true;
/* Match expressions of form &this->field. Those will most likely
combine with something upstream after inlining. */
@@ -1639,7 +1659,7 @@ eliminated_by_inlining_prob (gimple stmt)
if (TREE_CODE (op) == PARM_DECL)
rhs_free = true;
else if (TREE_CODE (op) == MEM_REF
- && unmodified_parm (stmt, TREE_OPERAND (op, 0)))
+ && unmodified_parm (stmt, TREE_OPERAND (op, 0), NULL))
rhs_free = true;
}
@@ -1652,7 +1672,7 @@ eliminated_by_inlining_prob (gimple stmt)
/* Reads of parameters passed by reference
expected to be free (i.e. optimized out after inlining). */
if (TREE_CODE (inner_rhs) == MEM_REF
- && unmodified_parm (stmt, TREE_OPERAND (inner_rhs, 0)))
+ && unmodified_parm (stmt, TREE_OPERAND (inner_rhs, 0), NULL))
rhs_free = true;
/* Copying parameter passed by reference into gimple register is
@@ -1693,7 +1713,7 @@ eliminated_by_inlining_prob (gimple stmt)
if (TREE_CODE (inner_lhs) == PARM_DECL
|| TREE_CODE (inner_lhs) == RESULT_DECL
|| (TREE_CODE (inner_lhs) == MEM_REF
- && (unmodified_parm (stmt, TREE_OPERAND (inner_lhs, 0))
+ && (unmodified_parm (stmt, TREE_OPERAND (inner_lhs, 0), NULL)
|| (TREE_CODE (TREE_OPERAND (inner_lhs, 0)) == SSA_NAME
&& SSA_NAME_VAR (TREE_OPERAND (inner_lhs, 0))
&& TREE_CODE (SSA_NAME_VAR (TREE_OPERAND
@@ -1723,6 +1743,7 @@ set_cond_stmt_execution_predicate (struct ipa_node_params *info,
{
gimple last;
tree op;
+ HOST_WIDE_INT size;
int index;
struct agg_position_info aggpos;
enum tree_code code, inverted_code;
@@ -1740,7 +1761,7 @@ set_cond_stmt_execution_predicate (struct ipa_node_params *info,
/* TODO: handle conditionals like
var = op0 < 4;
if (var != 0). */
- if (unmodified_parm_or_parm_agg_item (info, last, op, &index, &aggpos))
+ if (unmodified_parm_or_parm_agg_item (info, last, op, &index, &size, &aggpos))
{
code = gimple_cond_code (last);
inverted_code
@@ -1756,7 +1777,7 @@ set_cond_stmt_execution_predicate (struct ipa_node_params *info,
unordered one. Be sure it is not confused with NON_CONSTANT. */
if (this_code != ERROR_MARK)
{
- struct predicate p = add_condition (summary, index, &aggpos,
+ struct predicate p = add_condition (summary, index, size, &aggpos,
this_code,
gimple_cond_rhs (last));
e->aux = pool_alloc (edge_predicate_pool);
@@ -1786,11 +1807,11 @@ set_cond_stmt_execution_predicate (struct ipa_node_params *info,
return;
op2 = gimple_call_arg (set_stmt, 0);
if (!unmodified_parm_or_parm_agg_item
- (info, set_stmt, op2, &index, &aggpos))
+ (info, set_stmt, op2, &index, &size, &aggpos))
return;
FOR_EACH_EDGE (e, ei, bb->succs) if (e->flags & EDGE_FALSE_VALUE)
{
- struct predicate p = add_condition (summary, index, &aggpos,
+ struct predicate p = add_condition (summary, index, size, &aggpos,
IS_NOT_CONSTANT, NULL_TREE);
e->aux = pool_alloc (edge_predicate_pool);
*(struct predicate *) e->aux = p;
@@ -1809,6 +1830,7 @@ set_switch_stmt_execution_predicate (struct ipa_node_params *info,
gimple last;
tree op;
int index;
+ HOST_WIDE_INT size;
struct agg_position_info aggpos;
edge e;
edge_iterator ei;
@@ -1819,7 +1841,7 @@ set_switch_stmt_execution_predicate (struct ipa_node_params *info,
if (!last || gimple_code (last) != GIMPLE_SWITCH)
return;
op = gimple_switch_index (last);
- if (!unmodified_parm_or_parm_agg_item (info, last, op, &index, &aggpos))
+ if (!unmodified_parm_or_parm_agg_item (info, last, op, &index, &size, &aggpos))
return;
FOR_EACH_EDGE (e, ei, bb->succs)
@@ -1844,12 +1866,12 @@ set_switch_stmt_execution_predicate (struct ipa_node_params *info,
if (!min && !max)
p = true_predicate ();
else if (!max)
- p = add_condition (summary, index, &aggpos, EQ_EXPR, min);
+ p = add_condition (summary, index, size, &aggpos, EQ_EXPR, min);
else
{
struct predicate p1, p2;
- p1 = add_condition (summary, index, &aggpos, GE_EXPR, min);
- p2 = add_condition (summary, index, &aggpos, LE_EXPR, max);
+ p1 = add_condition (summary, index, size, &aggpos, GE_EXPR, min);
+ p2 = add_condition (summary, index, size, &aggpos, LE_EXPR, max);
p = and_predicates (summary->conds, &p1, &p2);
}
*(struct predicate *) e->aux
@@ -1949,13 +1971,14 @@ will_be_nonconstant_expr_predicate (struct ipa_node_params *info,
{
tree parm;
int index;
+ HOST_WIDE_INT size;
while (UNARY_CLASS_P (expr))
expr = TREE_OPERAND (expr, 0);
- parm = unmodified_parm (NULL, expr);
+ parm = unmodified_parm (NULL, expr, &size);
if (parm && (index = ipa_get_param_decl_index (info, parm)) >= 0)
- return add_condition (summary, index, NULL, CHANGED, NULL_TREE);
+ return add_condition (summary, index, size, NULL, CHANGED, NULL_TREE);
if (is_gimple_min_invariant (expr))
return false_predicate ();
if (TREE_CODE (expr) == SSA_NAME)
@@ -2016,6 +2039,7 @@ will_be_nonconstant_predicate (struct ipa_node_params *info,
struct predicate op_non_const;
bool is_load;
int base_index;
+ HOST_WIDE_INT size;
struct agg_position_info aggpos;
/* What statments might be optimized away
@@ -2039,7 +2063,7 @@ will_be_nonconstant_predicate (struct ipa_node_params *info,
tree op;
gcc_assert (gimple_assign_single_p (stmt));
op = gimple_assign_rhs1 (stmt);
- if (!unmodified_parm_or_parm_agg_item (info, stmt, op, &base_index,
+ if (!unmodified_parm_or_parm_agg_item (info, stmt, op, &base_index, &size,
&aggpos))
return p;
}
@@ -2050,7 +2074,7 @@ will_be_nonconstant_predicate (struct ipa_node_params *info,
adding conditionals. */
FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
{
- tree parm = unmodified_parm (stmt, use);
+ tree parm = unmodified_parm (stmt, use, NULL);
/* For arguments we can build a condition. */
if (parm && ipa_get_param_decl_index (info, parm) >= 0)
continue;
@@ -2065,18 +2089,18 @@ will_be_nonconstant_predicate (struct ipa_node_params *info,
if (is_load)
op_non_const =
- add_condition (summary, base_index, &aggpos, CHANGED, NULL);
+ add_condition (summary, base_index, size, &aggpos, CHANGED, NULL);
else
op_non_const = false_predicate ();
FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
{
- tree parm = unmodified_parm (stmt, use);
+ tree parm = unmodified_parm (stmt, use, &size);
int index;
if (parm && (index = ipa_get_param_decl_index (info, parm)) >= 0)
{
if (index != base_index)
- p = add_condition (summary, index, NULL, CHANGED, NULL_TREE);
+ p = add_condition (summary, index, size, NULL, CHANGED, NULL_TREE);
else
continue;
}
@@ -3300,7 +3324,8 @@ remap_predicate (struct inline_summary *info,
ap.by_ref = c->by_ref;
cond_predicate = add_condition (info,
operand_map[c->operand_num],
- &ap, c->code, c->val);
+ c->size, &ap, c->code,
+ c->val);
}
}
/* Fixed conditions remains same, construct single
@@ -4119,6 +4144,7 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data,
{
struct condition c;
c.operand_num = streamer_read_uhwi (&ib);
+ c.size = streamer_read_uhwi (&ib);
c.code = (enum tree_code) streamer_read_uhwi (&ib);
c.val = stream_read_tree (&ib, data_in);
bp = streamer_read_bitpack (&ib);
@@ -4280,6 +4306,7 @@ inline_write_summary (void)
for (i = 0; vec_safe_iterate (info->conds, i, &c); i++)
{
streamer_write_uhwi (ob, c->operand_num);
+ streamer_write_uhwi (ob, c->size);
streamer_write_uhwi (ob, c->code);
stream_write_tree (ob, c->val, true);
bp = bitpack_create (ob->main_stream);
diff --git a/gcc/ipa-inline.h b/gcc/ipa-inline.h
index 7860dec..110ed4f 100644
--- a/gcc/ipa-inline.h
+++ b/gcc/ipa-inline.h
@@ -32,6 +32,8 @@ struct GTY(()) condition
/* If agg_contents is set, this is the offset from which the used data was
loaded. */
HOST_WIDE_INT offset;
+ /* Size of the access reading the data (or the PARM_DECL SSA_NAME). */
+ HOST_WIDE_INT size;
tree val;
int operand_num;
ENUM_BITFIELD(tree_code) code : 16;
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index c3890f7..4840dda 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -971,10 +971,10 @@ ipa_load_from_parm_agg_1 (vec<ipa_param_descriptor> descriptors,
bool
ipa_load_from_parm_agg (struct ipa_node_params *info, gimple stmt,
tree op, int *index_p, HOST_WIDE_INT *offset_p,
- bool *by_ref_p)
+ HOST_WIDE_INT *size_p, bool *by_ref_p)
{
return ipa_load_from_parm_agg_1 (info->descriptors, NULL, stmt, op, index_p,
- offset_p, NULL, by_ref_p);
+ offset_p, size_p, by_ref_p);
}
/* Given that an actual argument is an SSA_NAME (given in NAME) and is a result
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 70185b2..5b3e8cd 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -594,7 +594,7 @@ void ipa_analyze_node (struct cgraph_node *);
tree ipa_find_agg_cst_for_param (struct ipa_agg_jump_function *, HOST_WIDE_INT,
bool);
bool ipa_load_from_parm_agg (struct ipa_node_params *, gimple, tree, int *,
- HOST_WIDE_INT *, bool *);
+ HOST_WIDE_INT *, HOST_WIDE_INT *, bool *);
/* Debugging interface. */
void ipa_print_node_params (FILE *, struct cgraph_node *node);
diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 51b1903..62a5fe0 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -141,7 +141,7 @@ along with GCC; see the file COPYING3. If not see
#define LTO_SECTION_NAME_PREFIX ".gnu.lto_"
#define LTO_major_version 3
-#define LTO_minor_version 0
+#define LTO_minor_version 1
typedef unsigned char lto_decl_flags_t;
diff --git a/gcc/testsuite/gcc.dg/ipa/pr70646.c b/gcc/testsuite/gcc.dg/ipa/pr70646.c
new file mode 100644
index 0000000..f85816e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr70646.c
@@ -0,0 +1,40 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#pragma GCC optimize("no-unit-at-a-time")
+
+typedef unsigned char u8;
+typedef unsigned long long u64;
+
+static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
+{
+ return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & (u64)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 56))) : __builtin_bswap64(*p));
+}
+
+static inline u64 wwn_to_u64(void *wwn)
+{
+ return __swab64p(wwn);
+}
+
+void __attribute__((noinline,noclone)) broken(u64* shost)
+{
+ u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
+ *shost = wwn_to_u64(node_name);
+}
+
+void __attribute__((noinline,noclone)) dummy(void)
+{
+ __builtin_abort();
+}
+
+int main(int argc, char* argv[])
+{
+ u64 v;
+
+ broken(&v);
+
+ if(v != (u64)-1)
+ __builtin_abort();
+
+ return 0;
+}
--
2.8.2
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-05-19 14:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-11 15:45 [PR 70646] Store size to inlining predicate conditions Martin Jambor
[not found] ` <8402b16805536e57e9c06ee1fe1992c0@suse.de>
2016-05-19 14:38 ` Martin Jambor
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).