> Sorry for the slow review. TBH I was hoping someone else would pick > it up, since (a) I'm not very familiar with this code, and (b) I don't > really agree with the way that the current code works. I'm not sure the > current dependency checking is safe, so I'm nervous about adding even > more cases to it. And it feels like the different ifcvt techniques are > not now well distinguished, so that they're beginning to overlap and > compete with one another. None of that is your fault, of course. :)
I might be to blame, at least partially :) The idea back then was to do it here because (1) it can handle cases the other part cannot and (2) its costing is based on sequence cost rather than just counting instructions. > This condition looks odd. A SET_SRC is never null. But more importantly, > continuing means "assume the best", and I don't think we should assume > the best for (say) an insn with two parallel sets. IIRC I didn't consider two parallel sets at all :/ > But I'm not sure which cases this code is trying to catch. Is it trying > to catch cases where seq2 "spontaneously" uses registers that happen to > overlap with cond? If so, then when does that happen? And if it does > happen, wouldn't the sequence also have to set the registers first? In order for sequence costing to be superior to just counting "conditional" instructions we need to make sure that as few redundant instructions as possible are present in the costed sequences. (redundant as in "will be removed in a subsequent pass"). So, originally, the backend would always be passed a comparison (set cc (cmp a b)). On e.g. s390 this would result in at least two redundant instructions per conditional move so costing was always wrong. When passing the backend the comparison "result" e.g. (cmp cc 0) there is no redundant instruction. Now, passing the comparison is useful as well when we want to turn a conditional move into a min/max in the backend. This, however, overwrites the initial condition result and any subsequent iterations must not pass the result but the comparison itself to the backend. In my first approach I hadn't considered several special cases which, of course, complicated things further down the line. All that said - maybe trying hard to get rid of later-redundant insns is not a good approach after all? A lot of complexity would go away if we counted emitted conditional instructions just like the other ifcvt part. Maybe a middle ground would be to cost the initial sequence as well as if + jmp and compare this against insn_cost of only the created conditional insns? In that case we might need to tighten/audit our preconditions in order not to accidentally allow cases that result in strictly worse code. But maybe that's an easier problem than what we are currently solving? Regards Robin
