[patch] missing COND_EXPR case in walk_stmt_load_store_addr_ops
Zbigniew Chamski
zbigniew.chamski@gmail.com
Mon Aug 16 14:35:00 GMT 2010
Hi Richard and all,
I don't have a general (pure trunk-based and portable) test case yet.
Given the current compilation flow, such a test case would require a
way to trigger 'execute_update_address_taken' in late GIMPLE passes.
The only way to do it is to have a portable builtin which is folded
late and triggers the creation of new statements (cf.
execute_fold_all_builtins() in tree-ssa-ccp.c and
update_call_from_tree() in tree-ssa-propagate.c). Any other
suggestions?
BTW, there was a glitch in my original patch: the second arg of
'visit_addr()' calls in 'stmt_visit_select_addr_ops()' should have
been wrapped in TREE_OPERAND(..., 0) Proof that this thing needs a
proper regression test...
Here's the corrected patch (retested on my experimental build and on
the r163224 trunk). I changed the leading comment to emphasize that
it's the operand of ADDR_EXPR that is supplied to 'visit_addr()'.
Zbigniew
Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c (revision 163224)
+++ gcc/gimple.c (working copy)
@@ -4746,6 +4746,45 @@
return NULL_TREE;
}
+
+/* For an assignment STMT which contains a selection expression on the RHS,
+ i.e.,
+
+ lhs = [cond_expr] a RELOP b ? x : y;
+
+ search the entire selection expression for occurrences of ADDR_EXPR, and
+ if found, apply VISIT_ADDR callback on STMT, the corresponding operand
+ of ADDR_EXPR and DATA. Return TRUE if any of the callback invocations
+ returned TRUE. */
+
+static inline bool
+stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
+ bool (*visit_addr)(gimple, tree, void *))
+{
+ bool ret = false;
+ tree condop1, condop2;
+ tree trueval, falseval;
+
+ condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
+ if (TREE_CODE (condop1) == ADDR_EXPR)
+ ret |= visit_addr (stmt, TREE_OPERAND (condop1, 0), data);
+
+ condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 1);
+ if (TREE_CODE (condop2) == ADDR_EXPR)
+ ret |= visit_addr (stmt, TREE_OPERAND (condop2, 0), data);
+
+ trueval = TREE_OPERAND (select_expr, 1);
+ if (TREE_CODE (trueval) == ADDR_EXPR)
+ ret |= visit_addr (stmt, TREE_OPERAND (trueval, 0), data);
+
+ falseval = TREE_OPERAND (select_expr, 2);
+ if (TREE_CODE (falseval) == ADDR_EXPR)
+ ret |= visit_addr (stmt, TREE_OPERAND (falseval, 0), data);
+
+ return ret;
+}
+
+
/* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
VISIT_ADDR if non-NULL on loads, store and address-taken operands
passing the STMT, the base of the operand and DATA to it. The base
@@ -4761,6 +4800,7 @@
{
bool ret = false;
unsigned i;
+
if (gimple_assign_single_p (stmt))
{
tree lhs, rhs;
@@ -4785,6 +4825,9 @@
&& TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
0), data);
+ else if (TREE_CODE (rhs) == COND_EXPR)
+ ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
+
lhs = gimple_assign_lhs (stmt);
if (TREE_CODE (lhs) == TARGET_MEM_REF
&& TMR_BASE (lhs) != NULL_TREE
2010/8/16 Richard Guenther <richard.guenther@gmail.com>:
> On Mon, Aug 16, 2010 at 1:57 PM, Zbigniew Chamski
> <zbigniew.chamski@gmail.com> wrote:
>> All,
>>
>> walk_stmt_load_store_ops does not handle the case of COND_EXPR on the
>> RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of
>> the form
>>
>> v = [cond_expr] a RELOP b : x ? y;
>>
>> This may result in an ICE at SSA verification time if/when the
>> ADDR_EXPRs inside the selection expression are not taken into account.
>> There is a corner case in libiberty (plus-demangle-template() in
>> cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable
>> 'pattern' ends up in a selection expression. A call to
>> 'execute_update_addresses_taken()' will miss it, resulting in an
>> "address taken, but ADDRESSABLE bit not set" error at SSA verification
>> time. This normally goes unnoticed because there are no calls to
>> 'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear
>> on the RHS much later, during LIM. However, the bug will be triggered
>> if/when execute_fold_all_builtins (or a custom pass) alters data
>> addressing and sets TODO_update_address_taken. In my case it was
>> uncovered by a custom pass.
>>
>> The root cause is in the logic of 'walk_stmt_load_store_ops' which
>> lacks the COND_EXPR case for simple assigns. A proposed patch is
>> below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu.
>>
>> Comments welcome!
>
> The patch is ok for trunk and the 4.5 branch (do you have a testcase?).
>
> Thanks,
> Richard.
>
>> Zbigniew
>>
>> Index: gcc/gimple.c
>> ===================================================================
>> --- gcc/gimple.c (revision 163224)
>> +++ gcc/gimple.c (working copy)
>> @@ -4746,6 +4746,45 @@
>> return NULL_TREE;
>> }
>>
>> +
>> +/* For an assignment STMT which contains a selection expression on the RHS,
>> + i.e.,
>> +
>> + lhs = [cond_expr] a RELOP b ? x : y;
>> +
>> + search the entire selection expression for occurrences of ADDR_EXPR, and
>> + if found, apply VISIT_ADDR callback on STMT, the corresponding ADDR_EXPR
>> + operand and DATA. Return TRUE if any of the callback invocations
>> + returned TRUE. */
>> +
>> +static inline bool
>> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
>> + bool (*visit_addr)(gimple, tree, void *))
>> +{
>> + bool ret = false;
>> + tree condop1, condop2;
>> + tree trueval, falseval;
>> +
>> + condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
>> + if (TREE_CODE (condop1) == ADDR_EXPR)
>> + ret |= visit_addr (stmt, condop1, data);
>> +
>> + condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 1), 0);
>> + if (TREE_CODE (condop2) == ADDR_EXPR)
>> + ret |= visit_addr (stmt, condop2, data);
>> +
>> + trueval = TREE_OPERAND (select_expr, 1);
>> + if (TREE_CODE (trueval) == ADDR_EXPR)
>> + ret |= visit_addr (stmt, trueval, data);
>> +
>> + falseval = TREE_OPERAND (select_expr, 2);
>> + if (TREE_CODE (falseval) == ADDR_EXPR)
>> + ret |= visit_addr (stmt, falseval, data);
>> +
>> + return ret;
>> +}
>> +
>> +
>> /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>> VISIT_ADDR if non-NULL on loads, store and address-taken operands
>> passing the STMT, the base of the operand and DATA to it. The base
>> @@ -4761,6 +4800,7 @@
>> {
>> bool ret = false;
>> unsigned i;
>> +
>> if (gimple_assign_single_p (stmt))
>> {
>> tree lhs, rhs;
>> @@ -4785,6 +4825,9 @@
>> && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>> ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>> 0), data);
>> + else if (TREE_CODE (rhs) == COND_EXPR)
>> + ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
>> +
>> lhs = gimple_assign_lhs (stmt);
>> if (TREE_CODE (lhs) == TARGET_MEM_REF
>> && TMR_BASE (lhs) != NULL_TREE
>>
>
--
dr Zbigniew Chamski - Infrasoft IT Solutions
ul. Oskara Kolberga 33, 09-407 PŁOCK, Poland
mobile: +48 668 773 916, phone: +48 24 262 0166
NIP 526-286-69-21, REGON: 141082454
More information about the Gcc-patches
mailing list