On Sat, Jan 9, 2016 at 12:20 AM, Jeff Law <l...@redhat.com> wrote: > On 01/08/2016 02:36 AM, Richard Biener wrote: >>> >>> My original patch did this for all binary operations. However, reviewing >>> the assembly code & dump files before/after it was pretty clear that >>> doing >>> this for arithmetic was losing (mostly in that we were missing CSE >>> opportunities). >> >> >> The missed CSE opportunities really are a CSE missed optimization in >> general with respect to two reassociatable chains with some (but not all) >> common operands. > > Agreed. It was a choice between expanding the scope and tracking down the > missed CSEs or nailing down 64910 without regressing and putting that the > missed CSEs on the TODO list. > > How about I build some testcases for the missed CSEs, xfailed with a BZ too, > so that we at least don't lose track of 'em. > >> So you are really trying to make RTL expansion see a different pattern? > > Yes, absolutely. The code we generate now does > > (x OP C) OP y > > which makes it bloody hard for the RTL/target bits to recognize bit > operations. After the patch we generate > > (x OP y) OP C > > Which the RTL & backends can easily see as bitwise operations. > > >> >> It seems to me that in this case using TER and get_def_for_expr / >> get_gimple_for_ssa_name >> should be used there. [or for future direction instruction selection >> should be performed on >> GIMPLE with some match-and-simplify patterns creating IFNs matching >> optabs if available] > > That seems backwards to me -- we have all the infrastructure in place in > reassoc.c we just make a terrible decision for the ordering of the operands. > In fact, the code is fine going into reassoc, it's reassoc that mucks things > up: > > Before reassoc we have: > > > ;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot > ;; prev block 0, next block 3, flags: (NEW, REACHABLE) > ;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE) > _4 = u_2(D) & w_3(D); > _7 = _4 & 32; > if (_7 != 0) > goto <bb 3>; > else > goto <bb 4>; > > > Which looks just like a bit test. Sadly reassoc comes around and turns it > into: > > ;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot > ;; prev block 0, next block 3, flags: (NEW, REACHABLE) > ;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE) > _8 = w_3(D) & 32; > _7 = _8 & u_2(D); > if (_7 != 0) > goto <bb 3>; > else > goto <bb 4>; > > > And we've lost.
Yeah, reassoc is largely about canonicalization. > Plus doing it in TER is almost certainly more complex than getting it right > in reassoc to begin with. I guess canonicalizing differently is ok but you'll still create ((a & b) & 1) & c then if you only change the above place. So I'm not sure what pattern the backend is looking for? Richard. > Jeff >
