| Message ID | 20240701025115.1265117-1-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/i386: CCOp cleanups | expand |
On Mon, Jul 1, 2024 at 4:51 AM Richard Henderson <richard.henderson@linaro.org> wrote: > While debugging #2413, I spent quite a bit of time trying to work > out if the CCOp value was incorrect. I think the following is a > worthwhile cleanup, isolating potential problems to asserts. Hi Richard, no objections at all to introducing more asserts. I think keeping the array is a better underlying implementation for cc_op_live() however. 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). 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. Paolo
On 7/1/24 10:48, Paolo Bonzini wrote: > On Mon, Jul 1, 2024 at 4:51 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> While debugging #2413, I spent quite a bit of time trying to work >> out if the CCOp value was incorrect. I think the following is a >> worthwhile cleanup, isolating potential problems to asserts. > > Hi Richard, > > 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. > > 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. If those extra enumerators are present, you either have to include them explicitly in the switch, which is less than desirable, or add a default case, which disables -Wswitch entirely. If you don't like abusing 4..7, then we can use 8..11, placing POPCNT at 10 or 11. > 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. r~
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
On 7/1/24 12:30, Paolo Bonzini wrote: > 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? Certainly. r~