| Message ID | 20170729235850.22180-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, Jul 29, 2017 at 7:58 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> 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 ?
I have no objection for this patch. If you want me to apply it, I am
happy to.
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.
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? 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.
This "try and error" process is very scary. The more I read on this
subject, what is the recommended best practices in the academia,
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.
I will apply it. My feel that it is still work in progress, not perfect yet.
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 30, 2017 at 5:38 PM, Christopher Li <sparse@chrisli.org> wrote: > On Sat, Jul 29, 2017 at 7:58 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 30, 2017 at 12:04 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > It's was draft/temptative patch for -rc5. > Thanks for the clarification. > > Please don't, I'm still working on it. Of course, I don't apply it now. I will wait for your git pull to apply. > 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. I totally agree. The revert is temporary bandage to buy us more time to fix the real problem. That is well understood. We still have the question to answer will RC5 and follow up official release have the revert or not. Currently I am leaning towards keep the revert for this release but I can be convinced otherwise. > 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 Yes, we all know "setne %r11, %r11, 0" is the big offender. We just don't have a clear picture why it get there. > 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. Sorry that came out wrong, I mean to said there is some more deeper issue. > But yes, this is quite annoying, especially with try_to_simplify_bb() > on which I already spend many hours on it. I also spend many hours on it haven't able to crack it yet. The best I can do is make it simplify the original huge test input as a much smaller test case. "Help me, Obi-Wan Kenobi. You're my only hope." > 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) We will revisit this after the release. I have my wish list as well. Right now let's solve this bug first. At least come to a decision what to do with it on the upcoming release. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/flow.c b/flow.c index d8198ce8d..fe16d78d4 100644 --- a/flow.c +++ b/flow.c @@ -79,6 +79,19 @@ static int bb_depends_on(struct basic_block *target, struct basic_block *src) return 0; } +/* + * This is only to be used by try_to_simplify_bb(). + * It really should be handled by bb_depends_on(), only + * that there is no liveness done on OP_PHI/OP_PHISRC. + * bb_depends_on() should return true if a phi + * is defined by src and used by target but it doesn't. + * So for now we add the explicit check. + */ +static int bb_uses_phi(struct basic_block *target, pseudo_t phi) +{ + return pseudo_in_list(target->needs, phi); +} + /* * This is only to be used by try_to_simplify_bb(). * It really should be handled by bb_depends_on(), only @@ -152,6 +165,8 @@ static int try_to_simplify_bb(struct basic_block *bb, struct instruction *first, target = true ? second->bb_true : second->bb_false; if (bb_depends_on(target, bb)) continue; + if (bb_uses_phi(target, first->target)) + continue; if (bb_defines_phi(bb, first)) continue; changed |= rewrite_branch(source, &br->bb_true, bb, target);