Re: Question on cfg_remove_useless_stmts_bb

Re: Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

    You might want to add a suitable comment while you're in there

Most certainly.

Re: Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

    the fact that it wasn't obvious to you is a good indication that I
    should have had more comments in that code.

Actually, there's still some confusion there.  Now that you've pointed it
out, from the way I read that code, VAL is always either a PARM_DECL or
a VAR_DECL.  So why test !TREE_CONSTANT (val)?

And why exclude a constant anyway

Re: Question on cfg_remove_useless_stmts_bb

[Jeffrey A Law]

On Wed, 2004-08-18 at 05:42, Richard Kenner wrote:
>     the fact that it wasn't obvious to you is a good indication that I
>     should have had more comments in that code.
> 
> Actually, there's still some confusion there.  Now that you've pointed it
> out, from the way I read that code, VAL is always either a PARM_DECL or
> a VAR_DECL.  So why test !TREE_CONSTANT (val)?
VAL can certainly be a constant.  Consider

if (a == 42)

Which should match:

      if (TREE_CODE (cond) == EQ_EXPR
          && (TREE_CODE (TREE_OPERAND (cond, 0)) == VAR_DECL
              || TREE_CODE (TREE_OPERAND (cond, 0)) == PARM_DECL)
          && (TREE_CODE (TREE_OPERAND (cond, 1)) == VAR_DECL
              || TREE_CODE (TREE_OPERAND (cond, 1)) == PARM_DECL
              || TREE_CONSTANT (TREE_OPERAND (cond, 1))))


FWIW, this is one of the things that could likely go away if we
don't translate trees out of SSA form -- this code exists merely
to clean up trivial redundancies created by the out-of-ssa
translation pass.

Jeff

Re: Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

    VAL can only be a _DECL node or a constant.  

I missed that part.  Thanks.

So I'll test and then commit that trivial change.

Re: Question on cfg_remove_useless_stmts_bb

[Jeffrey A Law]

On Tue, 2004-08-17 at 09:59, Richard Kenner wrote:
>     VAL can only be a _DECL node or a constant.  
> 
> I missed that part.  Thanks.
> 
> So I'll test and then commit that trivial change.
Sounds good.  You might want to add a suitable comment while you're
in there -- the fact that it wasn't obvious to you is a good
indication that I should have had more comments in that code.

jeff

Re: Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

      if (TREE_CODE (stmt) == ASM_EXPR
          || (TREE_CODE (stmt) == MODIFY_EXPR
              && (TREE_OPERAND (stmt, 0) == var
                  || TREE_CODE (stmt, 0) == val)))

    Ought to do the trick.

In this case, yes, but I'm not convinced that it's complete.  Couldn't
VAL be a more complex expression here and we have to verify that none of
its operands were modified?  That issue is basically the other motivation
for my query (the primary one being, of course, "how did this ever work?"!).

Re: Question on cfg_remove_useless_stmts_bb

[Jeffrey A Law]

On Tue, 2004-08-17 at 04:57, Richard Kenner wrote:
>       if (TREE_CODE (stmt) == ASM_EXPR
>           || (TREE_CODE (stmt) == MODIFY_EXPR
>               && (TREE_OPERAND (stmt, 0) == var
>                   || TREE_CODE (stmt, 0) == val)))
> 
>     Ought to do the trick.
> 
> In this case, yes, but I'm not convinced that it's complete.  Couldn't
> VAL be a more complex expression here and we have to verify that none of
> its operands were modified?  That issue is basically the other motivation
> for my query (the primary one being, of course, "how did this ever work?"!).
VAL can only be a _DECL node or a constant.  

  if (TREE_CODE (cond) == VAR_DECL || TREE_CODE (cond) == PARM_DECL)
    {
      var = cond;
      val = (bb->pred->flags & EDGE_FALSE_VALUE
             ? boolean_false_node : boolean_true_node);
    }
  else if (TREE_CODE (cond) == TRUTH_NOT_EXPR
           && (TREE_CODE (TREE_OPERAND (cond, 0)) == VAR_DECL
               || TREE_CODE (TREE_OPERAND (cond, 0)) == PARM_DECL))
    {
      var = TREE_OPERAND (cond, 0);
      val = (bb->pred->flags & EDGE_FALSE_VALUE
             ? boolean_true_node : boolean_false_node);
    }
  else
    {
      if (bb->pred->flags & EDGE_FALSE_VALUE)
        cond = invert_truthvalue (cond);
      if (TREE_CODE (cond) == EQ_EXPR
          && (TREE_CODE (TREE_OPERAND (cond, 0)) == VAR_DECL
              || TREE_CODE (TREE_OPERAND (cond, 0)) == PARM_DECL)
          && (TREE_CODE (TREE_OPERAND (cond, 1)) == VAR_DECL
              || TREE_CODE (TREE_OPERAND (cond, 1)) == PARM_DECL
              || TREE_CONSTANT (TREE_OPERAND (cond, 1))))
        {
          var = TREE_OPERAND (cond, 0);
          val = TREE_OPERAND (cond, 1);
        }
      else
        return;
 

Re: Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

    On a clean tree?  

No, on my tree.  But I still need to understand what's causing it,
hence my questions.

Question on cfg_remove_useless_stmts_bb

[Richard Kenner]

I'm getting a failure on c-torture/execute/930513-2.c with -O3 and what's
happening is that the out of ssa pass is looking at 

  if (j != i) goto <L1>; else goto <L2>;

    ...

<L2>:;
  i = i + 1;
  j = i;


The cfg_remove_useless_stmts_bb knows that J and I are equal.  So when it
gets to the "j = i" assignment, it deletes it.  The problem is that it
doesn't notice that "i = i + 1" clobbers the equality relation.

I don't think it's sufficient to test for an assignment to VAL here because
I think VAL can be an expression since we are no longer in GIMPLE, but
I'm not sure.

Hence my question.

I don't know why this test isn't failing for everybody, so I suspect this
is some sort of latent bug somehow.  (Or else I'm missing something,
which is quite possible.)

Re: Question on cfg_remove_useless_stmts_bb

[Diego Novillo]

On Fri, 2004-08-13 at 15:53, Richard Kenner wrote:
> I'm getting a failure on c-torture/execute/930513-2.c with -O3 and what's
> happening is that the out of ssa pass is looking at 
> 
On a clean tree?  This test worked fine last night on all of x86,
x86-64, alpha, ppc and ia64.


Diego.

Re: Question on cfg_remove_useless_stmts_bb

[Jeffrey A Law]

On Fri, 2004-08-13 at 13:53, Richard Kenner wrote:
> I'm getting a failure on c-torture/execute/930513-2.c with -O3 and what's
> happening is that the out of ssa pass is looking at 
> 
>   if (j != i) goto <L1>; else goto <L2>;
> 
>     ...
> 
> <L2>:;
>   i = i + 1;
>   j = i;
> 
> 
> The cfg_remove_useless_stmts_bb knows that J and I are equal.  So when it
> gets to the "j = i" assignment, it deletes it.  The problem is that it
> doesn't notice that "i = i + 1" clobbers the equality relation.
Ah, yes, the code ought to be checking that both I (VAR) & J (VAL) are
not clobbered.  I was pretty sure it did that, but I can't find the
code which verifies that VAL isn't clobbered.

I would think that changing
  if (TREE_CODE (stmt) == ASM_EXPR
      || (TREE_CODE (stmt) == MODIFY_EXPR
          && TREE_OPERAND (stmt, 0) == var)

To

  if (TREE_CODE (stmt) == ASM_EXPR
      || (TREE_CODE (stmt) == MODIFY_EXPR
          && (TREE_OPERAND (stmt, 0) == var
              || TREE_CODE (stmt, 0) == val)))

Ought to do the trick.

jeff