On 05/01/2018 02:42 AM, Marc Glisse wrote: > (I am not a reviewer, just commenting) But your comments are definitely appreciated!
> > On Fri, 9 Feb 2018, Roger Sayle wrote: > >> The following patch implements a number of __builtin_popcount related >> optimizations. >> (i) popcount(x) == 0 can be simplified to x==0, and popcount(x) != 0 to >> x!=0. >> (ii) popcount(x&1) can be simplified to x&1, and for unsigned x, >> popcount(x>>31) to x>>31. >> (iii) popcount (x&6) + popcount(y&16) can be simplified to >> popcount((x&6)|(y&16)) >> >> These may seem obscure transformations, but performing these types of >> POPCOUNT operations are often the performance critical steps in some >> cheminformatics applications. >> >> To implement the above transformations I've introduced the >> tree_nonzero_bits function, which is a tree-level version of rtlanal's >> nonzero_bits used by the RTL optimizers. > > I am wondering if this brings much, compared to just using > get_nonzero_bits (works on INTEGER_CST / SSA_NAME, matched by > with_possible_nonzero_bits). If we do decide to introduce this function, > we probably want to use it in other places that currently use > get_nonzero_bits as well... > >> + case NOP_EXPR: >> + case CONVERT_EXPR: > > We have CASE_CONVERT: for this pair. I've fixed this in Roger's patch. > >> + case LSHIFT_EXPR: >> + if (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST) > > Maybe check INTEGRAL_TYPE_P as well, like you did for PLUS_EXPR? Or was > that also unnecessary for PLUS_EXPR? While there may be cases where allowing an INTEGRAL_TYPE_P SSA_NAME for the shift count would be helpful, I think it'd be exceedingly rare. For operands of a PLUS_EXPR I think we're a lot more likely to be able to do something useful with an SSA_NAME argument. > >> + return wi::neg_p (arg1) >> + ? wi::rshift (nzbits, -arg1, TYPE_SIGN (type)) >> + : wi::lshift (nzbits, arg1); > > I can see that fold-const.c already does something like that. I am > surprised the sanitizer guys haven't asked that we just punt on negative > values instead. I'm leaving this as-is -- while I think negative shift counts are a bad idea, handling them in any other way is likely going to result in a real surprise in the result of the computation. > >> + /* popcount(X) + popcount(Y) is popcount(X|Y) when X&Y must be >> zero. */ >> + (simplify >> + (plus (popcount @0) (popcount @1)) > > We probably want :s on both popcount: if they are used in other places > than just this addition, it is likely cheaper not to introduce a new > call to popcount. Yea. I also verified the Roger's tests still work with the :s added. > >> + /* pocount(X) == 0 is X == 0, and related (in)equalities. */ > > po+p+count Fixed. > >> + (for cmp (le eq ne gt) >> + rep (eq eq ne ne) >> + (simplify >> + (cmp (popcount @0) zerop) > > Might as well use integer_zerop when we know we are dealing with integers. And fixed. I've also re-bootstrapped Roger's patch and will be committing it shortly. jeff
