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

Reply via email to