On 09/06/2017 03:26 AM, Jakub Jelinek wrote: > On Tue, Sep 05, 2017 at 11:21:48PM -0600, Jeff Law wrote: >> --- a/gcc/tree-ssa-reassoc.c >> +++ b/gcc/tree-ssa-reassoc.c >> @@ -5763,14 +5763,15 @@ reassociate_bb (basic_block bb) >> "Width = %d was chosen for reassociation\n", >> width); >> >> >> - /* For binary bit operations, if the last operand in >> - OPS is a constant, move it to the front. This >> - helps ensure that we generate (X & Y) & C rather >> - than (X & C) & Y. The former will often match >> - a canonical bit test when we get to RTL. */ >> - if ((rhs_code == BIT_AND_EXPR >> - || rhs_code == BIT_IOR_EXPR >> - || rhs_code == BIT_XOR_EXPR) >> + /* For binary bit operations, if there are at least 3 >> + operands and the last last operand in OPS is a constant, >> + move it to the front. This helps ensure that we generate >> + (X & Y) & C rather than (X & C) & Y. The former will >> + often match a canonical bit test when we get to RTL. */ >> + if (ops.length () != 2 > > So wouldn't it be clearer to write ops.length () > 2 ? > if (ops.length () == 0) > else if (ops.length () == 1) > come earlier, so it is the same thing, but might help the reader. Agreed. It's a trivial change, but I'll spin it with some other testing that's in progress.
> >> + && (rhs_code == BIT_AND_EXPR >> + || rhs_code == BIT_IOR_EXPR >> + || rhs_code == BIT_XOR_EXPR) >> && TREE_CODE (ops.last ()->op) == INTEGER_CST) >> std::swap (*ops[0], *ops[ops_num - 1]); > > Don't you then want to put the constant as second operand rather than first, > i.e. swap with *ops[1]? I don't think it matters in practice. I recall being concerned that we could end up with non-canonical gimple, but we don't AFAICT. > And doesn't swap_ops_for_binary_stmt undo it again? No. It doesn't. The rank checks get in the way IIRC. FWIW that's where I'd originally put the transformation, but Richi wanted it in the caller. Jeff
