Hi! On Mon, Jul 06, 2020 at 03:11:17AM +0200, Hans-Peter Nilsson wrote: > TL;DR: fixing a misdetection of what is a "simple move".
That is not a very correct characterisation of what this does :-) > Looking into performace degradation after de-cc0 for CRIS, I > noticed combine behaving badly; it changed a move and a > right-shift into two right-shifts, where the "combined" move was > not eliminated in later passes, and where the deficiency caused > an extra insn in a hot loop: crcu16 (and crcu32) in coremark. > > Before de-cc0, the insns input to combine looked like: > 33: r58:SI=r56:SI 0>>r48:SI > REG_DEAD r56:SI > 35: r37:HI=r58:SI#0 > and after: > 33: {r58:SI=r56:SI 0>>r48:SI;clobber dccr:CC;} > REG_DEAD r56:SI > REG_UNUSED dccr:CC > 35: {r37:HI=r58:SI#0;clobber dccr:CC;} > REG_UNUSED dccr:CC So a shift like this is at most as expensive as a move, on your target (or, in your backend, anyway ;-) ) > That is, there's always a parallel with a clobber of the > condition-codes register. Being a parallel, it's not an > is_just_move, but e.g. a single_set. > > For the de-cc0:ed "combination", it ended up as > 33: {r58:SI=r56:SI 0>>r48:SI;clobber dccr:CC;} > REG_UNUSED dccr:CC > 35: {r37:HI#0=r56:SI 0>>r48:SI;clobber dccr:CC;} > REG_DEAD r56:SI > REG_UNUSED dccr:CC > That is, a move and a shift turned into two shifts; the extra > shift is not eliminated by later passes, while the move was > (with cc0, and "will be again") leading to redundant > instructions. Which was the whole point of the is_just_move() thing, yes. Combine doesn't know most moves will be eliminated by RA (but many are useful to do have before RA, because it gives RA much more freedom). If a move is the same cost as a simple insn, doing two (say shift, like here) insns in parallel is cheaper on most machines than having a shift and a move sequentially. (2-2 combinations are helpful on single-scalar and even in-order machines as well, btw). > At first I thought this was due to parallel-ignorant old code > but the "guilty" change is actually pretty recent. Regarding a > parallel with a clobber not being "just" a move, there's only > the two adjacent callers seen in the patch (obviously with the > rename), and their use exactly matches to check that the > argument is a single_set which is a move. It's always applied > to an rtx_insn, so I changed the type and name to avoid the > "just" issue. I had to adjust the type when calling single_set. But it is *not* supposed to be the same as single_set. > I checked the original commit, c4c5ad1d6d1e1e a.k.a r263067 and > it seems parallels-as-sets were just overlooked and that this They were not. It causes regressions. That is why it has a different name, not something with "single set". "just_move" isn't a very good name, I couldn't come up with something better, it is a pretty complicated concept :-/ "i2_should_not_be_2_2_combined"? I'll rerun some testing to show this. It'll take a while. > patch appears to agree with the intent and the comment at the > use of i2_was_move and i3_was_move, which has a clause saying > "Also do this if we started with two insns neither of which was > a simple move". That says that we combine to 2 insns also if we started with only 2, but neither of those was just a move. > With this correction in place, the performance degradation > related to de-cc0 of the CRIS port as measured by coremark is > gone and turned into a small win. N.B.: there certainly is a > code difference in other hot functions, and the swing between > different functions is larger than this difference; to be dealt > with separately. > > Tested cris-elf, x86_64-linux, powerpc64le-linux, 2/3 through > aarch64-linux (unexpectedly slow). > > Ok to commit? No, sorry. One thing that could work is allowing (unused) clobbers of fixed registers (like you have here), or maybe some hook is needed to say this register is like a flags register, or similar. That should work for you, and not regress other targets, maybe even help a little? We'll see. Thanks, Segher