> -----Original Message----- > From: Kyrylo Tkachov <[email protected]> > Sent: Tuesday, September 1, 2026 2:55 PM > To: Jeff Law <[email protected]> > Cc: Cui, Lili <[email protected]>; [email protected] > Subject: Re: [PATCH] ifcvt: Extend noce_convert_multiple_sets to diamonds > [PR125557] > > > > > On 1 Sep 2026, at 08:45, Jeff Law <[email protected]> wrote: > > > > > > > > On 8/31/26 11:40 PM, Kyrylo Tkachov wrote: > >>> Sure, happy to -- once the discussion on the list has settled, I will > >>> evaluate > it on x86_64 and send the numbers. > >>> > >>> First, pre-evaluating the other arm into temporaries makes the diamond > case fall out very naturally. One thing I would like to suggest: the gate > here > requires one arm's live-out set to be a subset of the others, and I think that > limit can simply be dropped. > >>> > >>> Three steps: > >>> 1. The else arm's computations, evaluated unconditionally into fresh > temporaries. > >>> 2. one conditional move for each set of the then arm, selecting that > >>> set's > value against the value the else arm gave the same register -- or against the > register's incoming value, if the else arm leaves it alone. > >>> 3. one conditional move for each register that only the else arm sets, > selecting the register's incoming value against the temporary from step 1. > >>> > >>> Your patch already implements steps 1 and 2; only step 3 needs to be > added. It is the same kind of move as step 2, so both can share one loop -- > and once it is there, all the code that enforces the limit falls away: the > containment test, the "primary arm sets >= 2" rule and the arm swap. I have a > rough change on top of your patch attached. > >>> > >>> My general feeling is that we should let noce_convert_multiple_sets > convert as much as it can and leave it to the cost model to decide whether a > conversion is worth it, rather than restricting the shapes up front. > >>> > >>> There is a second reason I would like it to handle the general case: it > >>> brings > noce_convert_multiple_sets close enough to cond_move_process_if_block, > noce_convert_multiple_sets is the more capable of the two, so once it is no > longer restricted, the cases cond_move_process_if_block handles today would > be converted by it instead, and benefit from that. Not something for this > patch -- it just makes that step easier. > >>> > >>> > >> Thanks for the feedback. I think these extensions can be done cleaner as a > follow-up patch, so I’ve incorporated your patch into a patch 2/2 and sent out > the two for review. > >> Thanks your help! > > It's probably worth noting that converting multiple sets, particularly > > during > the first if-conversion pass may be a loss on some platforms, particularly > those > with weaker conditional move sequences (ex risc-v where a generalized > conditional move is 3 insns). It is often instead better to wait until after > various passes have cleaned things up and the block in question often ends up > with a single set. > > Thanks, is the 3-insn form somehow costed accordingly in the backend i.e. is > there some RTL-visible property we can use to tame the heuristic? > > > > > If passes are able to clean things up so that there's a single set we can > > often > use other sequences to generate more efficient code than a generalized > conditional move. > > > > There may be examples of this in bugzilla. I've certainly seen it happen on > multiple occasions. > > I’ll gather some aarch64 stats for SPEC2026 and I’m hoping Lili will help with > x86 evaluation so that we don’t over-if-convert. > Sure, I’ll evaluate the patch on x86 using SPEC2017 and 2026.
Lili > > > > And just to be clear, this patch hasn't been forgotten, it's just more > > complex > than most, so taking longer to work through. On a positive note I started > including it in my tester with Sunday's runs. I've seen a come quirks pop > since > then, but nothing I can concretely say is due to the if-conversion patch > though. > > Thanks, much appreciated. I have some ifcvt cleanup patches that I’ve been > working on that are staged behind this, hopefully they’ll pay back a small > part > of the extra complexity this one introduces ;) Kyrill > > > > > Jeff
