On Wed, Jul 26, 2017 at 5:00 AM, Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> wrote: > This is typical of situations where undefined pseudos are involved. > For example this is what Linus wrote on the subject some months ago: > > and.32 %r3 <- %r4, $-65 > or.32 %r4 <- %r3, $64 > > which *initializes* the pseudo %r4 in the second instruction, but uses > it in the first one. That's a classic pattern of uninitialized pseudos > in sparse exactly because of the SSA logic. > > But it is all internally consistent, and the simplification is > "correct". The simplification phase very much has a "garbage in, > garbage out" model. I think that model is wrong. It is not only "garbage in, garbage out". It cause sparse freak out due to incorrect SSA form like the wine dead loop bug. In the above example, %r4 was used *before* it's define, so there must exist an execution flow loop back to the beginning of this block. So the proper way should be some thing like: phi.32 %phi4 <- %phisrc1(VOID), %phisrc2(%r4) and.32 %r3 <- %phi4, $-65 or.32 %r4 <- %r3, $64 phisrc.32 %phiscrc2 <- %r4 Luc, are you fully back yet? If you are, please take a look at the wine compile dead loop bug. I already simplify the test source into minimal form. My guess is that it is cause by this invalid SSA form as well. BTW, gcc does not do this kind of invalid SSA form in their IR. We really should change this model. > > Yes, this is what was explained in the original patch description. OK we are on the same page on cause of the crazy programmer. Only different view on the SSA form. the BB early to avoid the "crazy programmer". >> That sounds more like a cover up rather than a proper fix up. > > Like explained in the original patch description, this situation with > a self-defined pseudo can happen in unreachable code *even* if the > pseudo was in fact defined if: > - the definition was done in a loop header > - the loop header was deleted by insert_branch() > * the definition is then removed, the pseudo become undefined > * it shouldn't matter since all it's use is now in dead code > * we don't know the that the loop is now dead code before > kill_unreachable_bbs() is called. > * therefore, kill_unreachable_bbs() must be called: > - after a branch have been deleted > - before simplify_one_memop() is called and is about to issue the > "crazy programmer" warning > BECAUSE THE "crazy programmer" WARNING SHOULD NOT BE ISSUED > IF THE CONCERNED CODE IS IN FACT DEAD CODE. I totally get that part. My point is that, the crazy programmer warning is an ill form of SSA to begin with. We might need to address that illegal SSA form as well. My impression is that the illegal SSA from cause the wine dead loop bug as well. > So, if you really prefer, you can call kill_unreachable_bbs() just before > the "crazy programmer" check instead of just after the branch delete > like it was done in my patch but calling kill_unreachable_bbs() > only after clean_up_insn() like done in your patch is wrong. I don't think it is wrong. Because even with your patch, there is other ways to get into "crazy programmer" situation. We need to address that. My point is that, "crazy programmer" is an ill form of SSA it shouldn't exist in the first place. > >> If we have the proper SSA form, the "crazy programmer" shouldn't >> pop up at all. > > See Linus explanation here above. I am not convinced that is a good enough reason to allow this kind of ill form of SSA. It save a phi instruction but cost a lot of trouble down the road. > I found absolutely incredible the amount of time already wasted > on this issue and it is even not closed yet. > I feel really tired and totally disgusted by all this. Luc, I hope you have a good rest during your time off sparse. Getting the compiler back end optimization right is hard and tedious. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
