On Fri, Nov 6, 2020 at 5:33 PM Luc Van Oostenryck
<luc.vanoostenryck@xxxxxxxxx> wrote:
>
> So, I think that the next questions are:
> * which selects should be moved up?
> * up to where should them be moved?
Honestly, it's not even clear they should be moved up. It could be
moved down too, particularly in this kind of case where there is only
one user of the result (ie move it down to just before the use).
Moving down to the use is in many ways is much easier than moving up,
because then you don't need to worry about whether the sources to the
select have been calculated yet. No need for any dominance analysis or
anything like that for that case.
But my personal guess is that by default we shouldn't try to
excessively move instructions unless we have an actual reason to do
so.
So I don't think your patch a good simplification in general.
Particularly the "move up" part, when you might just end up doing an
entirely unnecessary select that would normally have been done only in
some unusual conditional case.
Now, instead, I think the proper select simplification is to tie it
together with the conditional branch. Look at what I had after the
other select simplifications:
.L0:
<entry-point>
cbr %arg1, .L5, .L4
.L4:
call spin_lock
br .L5
.L5:
# call %r4 <- errno, %r1
select.32 %r6 <- %arg1, $0xffffffff, $0
cbr %arg1, .L6, .L2
.L2:
call spin_unlock
br .L6
.L6:
ret.32 %r6
and notice that
cbr %arg1, .L5, .L4
.L5:
select.32 %r6 <- %arg1, $0xffffffff, $0
sequence. In particular, notice how we have a conditional branch to a
"select" that has the *same* conditional as the branch!
So I think the *proper* fix is to not think of this case as "move the
select", but as a special case of branch following: the same way that
a conditional branch to another conditional branch can be simplified
if the condition is the same, you can simplify the case of
"conditional branch to select with the same condition".
That said, that "proper fix" looks a bit painful. My gut feel is that
we shouldn't have generated that "select" in the first place at all.
We should have kept it as a conditional branch, then done branch
following, and done the whole "turn a branch-over into a select" at a
later stage.
Have we done that if_convert_phi() simplification too early?
I didn't really follow the whole chain of how we got to this point.
Linus
