On Wed, Sep 5, 2012 at 2:36 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Wed, Sep 05, 2012 at 02:10:03PM -0700, Andrew Pinski wrote:
>> The problem here is the strlen optimization tries to remove a null
>> character store as we already have done it but it does it for a
>> volatile store which is not a valid thing to do.
>> This patch fixes the problem by ignoring statements which have
>> volatile operands.
>
> That should be caught by the !TREE_SIDE_EFFECTS (lhs) check a few lines
> later. Isn't the bug instead that remap_gimple_op_r copies over
> TREE_THIS_VOLATILE flag, but doesn't copy over TREE_SIDE_EFFECTS?
Yes it should be doing the copy. In fact I have touched this area before.
I will submit a new patch.
Thanks,
Andrew Pinski
>
>> * tree-ssa-strlen.c (strlen_optimize_stmt): Don't look at statements
>> which have volatile operands.
>>
>> testsuite/ChangeLog:
>> * gcc.dg/tree-ssa/strlen-1.c: New testcase.
>
>> Index: gcc/tree-ssa-strlen.c
>> ===================================================================
>> --- gcc/tree-ssa-strlen.c (revision 190993)
>> +++ gcc/tree-ssa-strlen.c (working copy)
>> @@ -1782,7 +1782,8 @@ strlen_optimize_stmt (gimple_stmt_iterat
>> break;
>> }
>> }
>> - else if (is_gimple_assign (stmt))
>> + else if (is_gimple_assign (stmt)
>> + && !gimple_has_volatile_ops (stmt))
>> {
>> tree lhs = gimple_assign_lhs (stmt);
>>
>> Index: gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c (revision 0)
>> +++ gcc/testsuite/gcc.dg/tree-ssa/strlen-1.c (revision 0)
>> @@ -0,0 +1,17 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +extern const unsigned long base;
>> +static inline void wreg(unsigned char val, unsigned long addr)
>> __attribute__((always_inline));
>> +static inline void wreg(unsigned char val, unsigned long addr)
>> +{
>> + *((volatile unsigned char *) (__SIZE_TYPE__) (base + addr)) = val;
>> +}
>> +void wreg_twice(void)
>> +{
>> + wreg(0, 42);
>> + wreg(0, 42);
>> +}
>> +
>> +/* We should not remove the second null character store to (base+42)
>> address. */
>> +/* { dg-final { scan-tree-dump-times " ={v} 0;" 2 "optimized" } } */
>> +/* { dg-final { cleanup-tree-dump "optimized" } } */
>
>
> Jakub
