On Sat, Nov 07, 2020 at 11:39:42AM -0800, Linus Torvalds wrote: > 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. It's just that in my experience, in these situation with context imbalance, very often 'something' had to move up, just before the 'if' where the imbalance is created. > 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. I totally agree with this. > 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! Yes, once the SEL(SEL(x,C,0),C,0) is handled this all simplify to .L0: <entry-point> select.32 %r6 <- %arg1, $0xffffffff, $0 cbr %arg1, .L6, .L4 .L4: call spin_lock # call %r4 <- errno, %r1 call spin_unlock br .L6 .L6: ret.32 %r6 but to me, this seems a very very special case, so not much interesting. > 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? Absolutely. I've often observed that it inhibits other simplifications related to conditionals. And, even when it doesn't, I think that many select simplifications would already be handled at SETxx + CBR level (like your previous SEL(x,x,0)). So, yes, the if-conversion is a very valuable simplification but I also think it's done too early. And there are some other simplifications that are valuable but only if not done too early (sorry, I've no concrete examples at hand). It slowly begins time to organize the optimizations in several phases. -- Luc
