Re: [PATCH] Replace has_single_use guards in store-merging

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi Jakub,

On 9 November 2017 at 13:58, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 8 Nov 2017, Jakub Jelinek wrote:
>
>> On Wed, Nov 08, 2017 at 04:20:15PM +0100, Richard Biener wrote:
>> > Can't you simply use
>> >
>> >    unsigned ret = 0;
>> >    FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
>> >      if (!has_single_use (op))
>> >        ++ret;
>> >    return ret;
>> >
>> > ?  Not sure if the bit_not_p handling is required.
>>
>> Consider the case with most possible statements:
>>   _1 = load1;
>>   _2 = load2;
>>   _3 = ~_1;
>>   _4 = ~_2;
>>   _5 = _3 ^ _4;
>>   store0 = _5;
>> All these statements construct the value for the store, and if we remove
>> the old stores and add new stores we'll need similar statements for each
>> of the new stores.  What the function is attempting to compute how many
>> of these original statements will be not DCEd.
>> If _5 has multiple uses, then we'll need all of them, so 5 stmts not
>> being DCEd.  If _5 has single use, but _3 (and/or _4) has multiple uses,
>> we'll need the corresponding loads in addition to the BIT_NOT_EXPR
>> statement(s).  If only _1 (and/or _2) has multiple uses, we'll need
>> the load(s) but nothing else.
>> So, there isn't a single stmt I could FOR_EACH_SSA_TREE_OPERAND on.
>> For BIT_{AND,IOR,XOR}_EXPR doing it just on that stmt would be too rough
>> approximation and would miss the case when the bitwise binary op result
>> is used.
>
> Hmm, I see.
>
>> > It doesn't seem you handle multi-uses of the BIT_*_EXPR results
>> > itself?  Or does it only handle multi-uses of the BIT_*_EXPR
>> > but not the feeding loads?
>>
>> I believe I handle all those precisely above (the only reason I've talked
>> about aproximation is that bit field loads/stores are counted as one stmt
>> and the masking added for handling multiple semi-adjacent bitfield
>> loads/stores aren't counted either).
>>
>> Given the above example:
>>       if (!has_single_use (gimple_assign_rhs1 (stmt)))
>>       {
>>         ret += 1 + info->ops[0].bit_not_p;
>>         if (info->ops[1].base_addr)
>>           ret += 1 + info->ops[1].bit_not_p;
>>         return ret + 1;
>>       }
>> Above should handle the _5 multiple uses case (the first operand is guaranteed
>> by the discovery code to be a possibly negated load).
>>       stmt = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (stmt));
>>       /* stmt is now the BIT_*_EXPR.  */
>>       if (!has_single_use (gimple_assign_rhs1 (stmt)))
>>       ret += 1 + info->ops[0].bit_not_p;
>> Above should handle the _3 multiple uses.
>>       else if (info->ops[0].bit_not_p)
>>       {
>>         gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (stmt));
>>         if (!has_single_use (gimple_assign_rhs1 (stmt2)))
>>           ++ret;
>>       }
>> Above should handle multiple uses of _1.
>>       if (info->ops[1].base_addr == NULL_TREE)
>>       return ret;
>> is an early return for the case when second argument to BIT_*_EXPR is
>> constant.
>>       if (!has_single_use (gimple_assign_rhs2 (stmt)))
>>       ret += 1 + info->ops[1].bit_not_p;
>> Above should handle the _4 multiple uses.
>>       else if (info->ops[1].bit_not_p)
>>       {
>>         gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs2 (stmt));
>>         if (!has_single_use (gimple_assign_rhs1 (stmt2)))
>>           ++ret;
>>       }
>> Above should handle the _2 multiple uses.
>>
>> And for another example like:
>>   _6 = load1;
>>   _7 = ~_6;
>>   store0 = _7;
>>       if (!has_single_use (gimple_assign_rhs1 (stmt)))
>>       return 1 + info->ops[0].bit_not_p;
>> Above should handle _7 multiple uses
>>       else if (info->ops[0].bit_not_p)
>>       {
>>         stmt = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (stmt));
>>         if (!has_single_use (gimple_assign_rhs1 (stmt)))
>>           return 1;
>>       }
>> Above should handle _6 multiple uses.
>
> Thanks for the explanation, it's now more clear what the code does.
>
> The patch is ok.  (and hopefully makes the PGO bootstrap issue latent
> again - heh)
>


I've noticed that this patch (r254579) introduces an ICE on aarch64:
    gcc.target/aarch64/vect-compile.c (internal compiler error)
    gcc.target/aarch64/vect.c (internal compiler error)


during GIMPLE pass: store-merging
In file included from /gcc/testsuite/gcc.target/aarch64/vect.c:5:0:
/gcc/testsuite/gcc.target/aarch64/vect.x: In function 'test_orn':
/gcc/testsuite/gcc.target/aarch64/vect.x:7:6: internal compiler error:
tree check: expected ssa_name, have mem_ref in has_single_use, at
ssa-iterators.h:400
0x547c93 tree_check_failed(tree_node const*, char const*, int, char const*, ...)
        /gcc/tree.c:9098
0x120a79d tree_check
        /gcc/tree.h:3344
0x120a79d has_single_use
        /gcc/ssa-iterators.h:400
0x120bb67 count_multiple_uses
        /gcc/gimple-ssa-store-merging.c:1413
0x120bd81 split_group
        /gcc/gimple-ssa-store-merging.c:1490
0x120c890 output_merged_store
        /gcc/gimple-ssa-store-merging.c:1699
0x120f07f output_merged_stores
        /gcc/gimple-ssa-store-merging.c:2023
0x120f07f terminate_and_process_chain
        /gcc/gimple-ssa-store-merging.c:2061
0x120f07f terminate_and_release_chain
        /gcc/gimple-ssa-store-merging.c:986
0x120f8b7 terminate_all_aliasing_chains
        /gcc/gimple-ssa-store-merging.c:969
0x12107d6 execute
        /gcc/gimple-ssa-store-merging.c:2454

Thanks,

Christophe


> Thanks,
> Richard.