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.
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.
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.
Jeff
