Re: Removing operand normalizaiton in get_expr_operands

[Jeffrey A Law <law at redhat dot com>]

On Wed, 2005-08-24 at 13:22 -0400, Daniel Berlin wrote:
> Currently, get_expr_operands "renormalizes" the actual tree operands on
> it's own whim , such that if you call update_stmt on something like "a +
> c", it may be "c + a" after the call to update_stmt.
> This is not the same as sorting the use operands, vuses, etc, which is
> fine.
> 
> This is actually modifying the expression, so that TREE_OPERAND (rhs, 0)
> before the call is not necessarily TREE_OPERAND (rhs, 0) after the call.
> 
> In the reassociation rewrite (which rewrites the resasoc pass so that it
> catches all the things we've been looking for, like a & b & ~a & ~b,
> etc), it's very nice if we can reorder the operands temporarily so that
> the leaves are all to one side.
> We also need to keep the immediate uses up to date as we move things
> around, so that we have to call update_stmt.
> 
> If update_stmt reorders things, then you have to keep checking whether
> the  thing you thought was a leaf before update_stmt, is still in the
> same place, and reverse your operands, recurse on the correct side, etc.
> It's quite ugly.  This ugliness happens during both linearization, and
> later rewriting.
> 
> So I proposed to Andrew Macleod that we have an update_stmt interface
> that lets us choose not to resort the actual operands, and i'll just
> resort the expressions i touch when we are done.  He went me one better
> and said we should just remove the code that is swapping the actual
> operands around.
> 
> He didn't seem to remember why it was added.
> 
> Here's why i agree that it should be removed entirely:
> 
> 1. operand_equal_p already takes into account commutative operations.
> 2. iterative_hash_expr does as well.
> 3. None of the optimizations are trying to use simple pointer
> equivalence on actual binary operator arguments, AFAICT (Andrew said DOM
> might, but i don't see where).
> 4. Calling update_stmt on a statement should not actually change the
> statement itself.  It seems completely counter-intuitive.
> 5. Fold will already do this, and we should be using it where necessary
> anyway.
> 6. It wastes some small amount of time to do this :)
> 
> The counter arguments i can see are:
> 1. Auto-canonicalization is theoretically nice.
> 
> Which is good and all, but it only helps if you are trying to see if the
> expressions are exactly the same, and 
> 1. we use operand_equal_p for that anyway, because it knows more than we
> do about which operands are really equal.
> 2. fold will put them in the right order anyway, and things that modify
> or generate operations generally call fold.
> 
> So my proposal is to simply remove the code that does this in
> tree-ssa-operands.c:get_expr_operands (search for tree_swap_operands to
> see it).
The auto-canonicalization does present some problems.  No doubt about
that.  However, I was added specifically because it was allowing us
to eliminate more useless crud.  IIRC it was comparison elimination
that primarily benefited from auto canonicalization.

I don't recall any changes which would make the auto canonicalization
no longer necessary, but if you can show no ill effects before/after
removing auto canonicalization, then I won't object.


jeff