Am 12.07.24 um 18:40 schrieb Jeff Law:
On 7/10/24 3:05 AM, Georg-Johann Lay wrote:
The previous change to avr.md was several days ago, and should not
interfere with this one. Anyway, I rebased the patch against
master and attached it below. The patch is atop the ref in the patch
file name : https://gcc.gnu.org/r15-1935
I'll throw this version in. While avr isn't deeply tested additional
testing never hurts.
It looks like avr exposes the CC register early, creating references
to it during expansion to RTL. Presumably this means you've got a
reasonable way to reload values, particularly address arithmetic
without impacting the CC state?
No. CC only comes into existence after reload in .split2 and in some
cases in .avr-ifelse. reloads may clobber CC, so there is no way
to split cbranch insn prior to relaod.
Clearly I missed all those reload_completed checks. Thanks for clarifying.
It looks like you're relying heavily on peep2 patterns. Did you
explore using cmpelim?
Some question I have:
- compare-elim.cc states that the comparisons must look like
[(set (reg:CC) (compare:CC (reg) (reg_or_immediate)))]
which is not always the case, in particular the patterns
may have clobbers. compare-elim uses single_set, which
is true for the pattern with a clobber / scratch. However,
presence / absence of a clobber reg has to be taken into
account when deciding whether a transformation into cc
other than CCmode is possible. compare-elim only supplies
the SET_SRC, but not the scratch_operand to SELECT_CC_MODE.
We could probably fix that. I don't think we've had a target where the
clobbered object affects CC like this. Could we perhaps pass in the
full insn? That would require fixing the various targets that already
use the compare-elim infrastructure, but I think it'd be a pretty
mechanical change.
So basically affect all targets. I definitely cannot do that,
in particular the required testing.
- The internals says that some optimizations / transforms
will happen prior to reload (e.g. .combine). This is not
possible since CCmode exists only since .split2.
- Insn combine may have transformed some comparisons, e.g.
sign test are represented as a zero_extract + skip like
in the sbrx_branch insns. This means that compares might
have a non-canonical form at .cmpelim. But a CCNmode
compare is better still than an sbrx_branch. Moreover,
CANONICALIZE_COMPARISON also runs already at insn combine,
so that compares have a form not recognized by cmpelim.
There's probably going to be some cases we can't necessarily handle due
to intricate target dependencies. That's fine. I'm more concerned
about whether or not we're utilizing the existing infrastructure. If we
can use the generic infrastructure we should. If we can use the generic
infrastructure after making some improvements we should. If the
problems aren't tractable with the generic infrastructure, then a
bespoke target solution may be the only path forward.
Well, what matters in the end is the quality of the generated code.
Which doesn't mean I think how the code is written does not matter.
After all, peep2 is also an existing infrastructure. Yes, the code
is a bit verbose, but it gives a very well control over what is
happening, as it runs just a couple of passes after .split2.
Plus, there are cases where peephole 2 may provide a scratch, that's
something cmpelim can't do (to my knowledge).
- Comparisons may look like if (0 <code> ref) which is
a form not supported by compare-elim.
That's not canonical RTL. If the backend is generating that it probably
needs to be fixed. Various passes assume the constant will be in the
other position.
It is generated deliberately during CANONICALIZE_COMPARISON.
Reason is that it is possible to swap comparison operands with
zero costs, but that does not apply to branch instructions:
Not all branches are supported natively, for example there is GE,
but GT has to be emulated by 2 instructions. Which means
swapping conditions may add or remove costs.
This works well since beginning of time. Maybe it could be supported
with cmpelim by doubling the number of CCmodes, so things are getting
complicated, or at least annoying.
Moreover, as far as I can tell, cmpelim cannot convert code like
cc = compare (a, b)
if (cc == 0) goto x
cc = compare (a, b)
if (cc > 0) goto y
to
cc = compare (a, b)
if (cc == 0) goto x
if (cc >= 0) goto y
notice the 2nd operator changed, which is valid because >= is in the
wake of ==. cmpelim only provides local information and would come up
with
cc = compare (a, b)
if (cc == 0) goto x
if (cc > 0) goto y // suboptimal, not enough context known.
so cmpelim would require a device that tells which assertions hold
when a comparison is handled. For example, in the 1st compare there
is no assertion, but in the 2nf we know that always cc != 0 due to the
branch.
So I guess mainly due to the cmpelim (non-)alternative?
Largely yes. I'll try to take a look at this now that I've got more
background info. Thanks.
jeff
I must admit that I don't fell good about when you are putting loads
of effort into this...
My preferred solution is still peep2, or at least stuff that's in the
avr realm. My experience is that when problems are popping up, in
particular on the efficiency side, avr is not important enough and
falls off the cliff...
What about starting a new md file for this stuff? Might be easier
to follow than the current 10000 LOC of avr.md (there's already
3 md's to date, one additional for DImode and one for the fixed-point
stuff).
Johann
