Re: [PATCH 0/5] target/i386: CCOp cleanups

[Paolo Bonzini]

On Mon, Jul 1, 2024 at 9:05 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> > no objections at all to introducing more asserts. I think keeping the
> > array is a better underlying implementation for cc_op_live() however.
>
> Hmm.  I had an implementation that would detect missing entries at runtime, 
> but this one
> detects missing entries at compile time.

But how common is it to add new CCOps? I find the array more readable
(and it can also be printed quickly from gdb), while the switch
statement optimizes for a really rare case.

> > I'm also not very fond of mixing "sized" and "unsized" CCOps in the
> > 4..7 range, there's no real reason why CC_OP_DYNAMIC and CC_OP_CLR
> > must be close to CC_OP_EFLAGS and the ADCOX CCOps.  I also think it's
> > clearer to keep CC_OP_POPCNT[BWLQ] (even though in practice only one
> > will be used because popcnt needs zero extension anyway).
>
> My objection to keeping the unused POPCNT* enumerators is that it interferes 
> with proper
> cooperation with -Wswitch, to diagnose missing enumerators.  This is also why 
> I removed
> CC_OP_NB.

Yes, I agree with removing CC_OP_NB. However the unused POPCNT[BWLQ]
can be implemented trivially (they're all the same).

> > As an aside, I'm wondering if CC_OP_CLR is particularly important; I
> > expect "xor reg, reg" to be followed by more ALU operations most of
> > the time and to not be followed by a jump, so it only saves a spill if
> > xor reg, reg is followed by a lot or store. If gen_XOR used either
> > CC_OP_LOGICn or CC_OP_EFLAGS for "xor reg, reg", the values in
> > decode->cc_op_* (CC_OP_DST=0 for CC_OP_LOGICn; CC_OP_SRC=CC_Z|CC_P for
> > CC_OP_EFLAGS) would be constant and wouldn't add to register pressure.
>
> You could easily be right.  Improvements to tcg in the last 11 years may have 
> made it
> redundant, or it might have been wishful thinking even at the time.

Maybe. Just looking at the last couple years, with PCREL the cost of
translation has decreased substantially, and with the new decoder the
number of tcg ops has increased a bit. In both cases that means that
counting the tcg ops becomes less important.

BTW I found an easy way to implement X86_SPECIAL_BitTest without
crashes (just use cpu_regs[op->n] when computing the displacement
since you cannot have ah/bh/ch/dh). But I think it will be for 9.2.
Maybe these patches can wait too?

Paolo