On Sun, Jul 30, 2017 at 5:38 PM, Christopher Li <sparse@xxxxxxxxxxx> wrote: > On Sat, Jul 29, 2017 at 7:58 PM, Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> wrote: >> This is a temptative patch for the wine infinitive loop. > > First thing first, is this patch for this RC5 release or after the release ? It's was draft/temptative patch for -rc5. > I have no objection for this patch. If you want me to apply it, I am > happy to. Please don't, I'm still working on it. > That being said, I am still not very comfortable with this situation > even with the patch applied. The patch is definitely an improvement, it > stop at least one bug situation. I am not able to reason this patch to make > sure that is enough. I don't like at all this situation also. The problem is quite complex and deep. This patch seems to solve the problem and (at least something similar) is needed but my first analysis was also right and thus something similar is still needed for simplify_branch_branch(). Also, the revert would not be a good solution because it would only avoid to trigger the problem on this input but won't solve anything. > Yes, we observe from there is a case simplify a branch with bb has > some phi cause problem. > > Does all phi cause problem? Or only phi has uninitialized value cause > problem? The problem is unrelated to undefined *var* but these wrong branch simplification bypass some pseudo definition which, in a way, become undefined on some paths which then create the problem you saw with setne %r11, %r11, 0 > Will adding this check enough for all possible input source code? > There is a lot of unknown we can't answer. There is not a good theory behind > it we can reason and prove it one way or the other. That is what I feel > uncomfortable about, the unknown. You're not alone. > This "try and error" process is very scary. Well, I don't see things exactly the same. For me, there is a code base, there are bugs in the code, we discover the bugs and we fix them. But yes, this is quite annoying, especially with try_to_simplify_bb() on which I already spend many hours on it. > The more I realized sparse has been doing a lot of stuff relate to optimization > wrong. The phi node placement as you point out is a big one, there are others. > That is my general observation, not reason to against you patch though. > That is way beyond the scope of this patch any way. The two big things that need to be fixed regarding optimization are: - the placement of the phi-node - associate each phi's source to the phi-node's parent (Linus may object on this, he at least objected 10 years ago) The last point will then allow tfix some issues I'm aware of regarding liveness and make things much easier when manipulating phis. -- Luc -- 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
