| Message ID | 201006150009.13542.paul@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
Paul Brook wrote: > 2010-06-14 Paul Brook <paul@codesourcery.com> > > gcc/ > * cse.c (cse_process_notes_1): Call simplify_rtx is a substitution > was made. I think this qualifies as near-obvious. OK.
On Tue, Jun 15, 2010 at 1:41 AM, Mark Mitchell <mark@codesourcery.com> wrote: > Paul Brook wrote: > >> 2010-06-14 Paul Brook <paul@codesourcery.com> >> >> gcc/ >> * cse.c (cse_process_notes_1): Call simplify_rtx is a substitution >> was made. > > I think this qualifies as near-obvious. OK. How is this obvious? 1. It means validate_change validates something that is not canonical RTL. 2. You are canonicalizing something that is in a REG-note. How does this non-canonical note content escape from the note into an INSN? Is that not the point where this bug should be fixed? It makes me suspect that this change only papers over a bug elsewhere, where a note replacement in an insn is not validated properly. Ciao! Steven
Steven Bosscher wrote: >>> 2010-06-14 Paul Brook <paul@codesourcery.com> >>> >>> gcc/ >>> * cse.c (cse_process_notes_1): Call simplify_rtx is a substitution >>> was made. > How is this obvious? > It makes me suspect that this change only papers over a bug elsewhere, > where a note replacement in an insn is not validated properly. I think we worry too much about that. Or, rather, we confuse *suboptimal* patches with *wrong* patches. A wrong patch is one that will make the compiler generate wrong code, or slow code, or cause an ICE, or do some other user-observable bad thing. A suboptimal patch is one for which there exists a superior implementation technique, possibly involving some much larger reworking of the sourcebase, that would produce a solution to more problems, a more elegant solution to the same problem, or in some other way be superior. Of course, I am all for superior patches, good infrastructure, clean-ups, and so forth. But, it seemed clear to me that (a) validate_change wasn't in the business of making canonical RTL, and (b) simplify_rtx was, and (c) this patch made the compiler better from a user-observable point of view, in that it eliminated crashes, and (d) there had been some previous discussion suggesting this approach, and (e) I couldn't see any user-observable harm it would do, and (f) it was a small local change. I think that should be good enough. Now, of course, if you have an idea about how to do it better, I think that's great! A more general, more elegant solution would be terrific. But, I don't want perfect to be the enemy of good.
On 06/15/2010 04:08 PM, Mark Mitchell wrote: > Of course, I am all for superior patches, good infrastructure, > clean-ups, and so forth. But, it seemed clear to me that (a) > validate_change wasn't in the business of making canonical RTL, and > (b) simplify_rtx was For that matter, canonicalize_change_group also is, and is more directly related to validate_change. > and (c) this patch made the compiler better from a user-observable > point of view, in that it eliminated crashes, Without knowing the real bug, you cannot know if there are other identical crashes waiting to bite you. > and (d) there had been some previous discussion suggesting this > approach Where? > I think that should be good enough. Now, of course, if you have an > idea about how to do it better, I think that's great! A more > general, more elegant solution would be terrific. I gave a patch and Steven outlined another idea that might be even better. Again, I cannot say if Steven's idea would work without knowing what the real bug is. > But, I don't want perfect to be the enemy of good. But "papering over the real bug" _is_ the enemy of "good". That is something clearly undesirable, not just suboptimal, especially at the beginning of stage1. I'm not opposing the patch at all costs, but definitely the bug hasn't been analyzed thoroughly enough. Paolo
On Tue, Jun 15, 2010 at 5:32 PM, Paolo Bonzini <bonzini@gnu.org> wrote: > On 06/15/2010 04:08 PM, Mark Mitchell wrote: >> >> Of course, I am all for superior patches, good infrastructure, >> clean-ups, and so forth. But, it seemed clear to me that (a) >> validate_change wasn't in the business of making canonical RTL, and >> (b) simplify_rtx was > > For that matter, canonicalize_change_group also is, and is more directly > related to validate_change. > >> and (c) this patch made the compiler better from a user-observable >> point of view, in that it eliminated crashes, > > Without knowing the real bug, you cannot know if there are other > identical crashes waiting to bite you. > >> and (d) there had been some previous discussion suggesting this >> approach > > Where? > >> I think that should be good enough. Now, of course, if you have an >> idea about how to do it better, I think that's great! A more >> general, more elegant solution would be terrific. > > I gave a patch and Steven outlined another idea that might be even > better. Again, I cannot say if Steven's idea would work without knowing > what the real bug is. > >> But, I don't want perfect to be the enemy of good. > > But "papering over the real bug" _is_ the enemy of "good". That is > something clearly undesirable, not just suboptimal, especially at the > beginning of stage1. > > I'm not opposing the patch at all costs, but definitely the bug hasn't > been analyzed thoroughly enough. And apparently we're now completely stalled. Paul, are you still working on this? Is there anything you can file in bugzilla, at least, so this discussion will not get lost? Ciao! Steven
Steven Bosscher wrote: >> I'm not opposing the patch at all costs, but definitely the bug hasn't >> been analyzed thoroughly enough. > > And apparently we're now completely stalled. Paul, are you still > working on this? Is there anything you can file in bugzilla, at least, > so this discussion will not get lost? We do seem badly stuck. But, I don't understand, at this point, what we can do to get unstuck. I've just reread the thread and I don't see a clear direction about what form of patch would be acceptable here. Or, if we don't understand that yet, what information we need next. Paolo, Steven, would you please comment? Thanks,
On 08/05/2010 12:27 AM, Mark Mitchell wrote: > Steven Bosscher wrote: > >>> I'm not opposing the patch at all costs, but definitely the bug hasn't >>> been analyzed thoroughly enough. >> >> And apparently we're now completely stalled. Paul, are you still >> working on this? Is there anything you can file in bugzilla, at least, >> so this discussion will not get lost? > > We do seem badly stuck. > > But, I don't understand, at this point, what we can do to get unstuck. > I've just reread the thread and I don't see a clear direction about what > form of patch would be acceptable here. Or, if we don't understand that > yet, what information we need next. We need to know why the non-canonical note gets into an insn. This is a much more serious bug than a non-canonical note. After that, or if that fails, I posted another patch in the thread and I would like to understand if this patch also fixes the bug. Paolo
Paolo Bonzini wrote: > We need to know why the non-canonical note gets into an insn. This is a > much more serious bug than a non-canonical note. Paul, can you debug the compiler to figure out that question? Thanks,
On Thu, Aug 5, 2010 at 2:16 AM, Mark Mitchell <mark@codesourcery.com> wrote: > Paolo Bonzini wrote: > >> We need to know why the non-canonical note gets into an insn. This is a >> much more serious bug than a non-canonical note. > > Paul, can you debug the compiler to figure out that question? This issue has gotten stuck again. Ping? Ciao! Steven
Index: gcc/cse.c =================================================================== --- gcc/cse.c (revision 160724) +++ gcc/cse.c (working copy) @@ -5989,6 +5989,7 @@ cse_process_notes_1 (rtx x, rtx object, enum rtx_code code = GET_CODE (x); const char *fmt = GET_RTX_FORMAT (code); int i; + bool did_change = false; switch (code) { @@ -6057,7 +6058,18 @@ cse_process_notes_1 (rtx x, rtx object, for (i = 0; i < GET_RTX_LENGTH (code); i++) if (fmt[i] == 'e') validate_change (object, &XEXP (x, i), - cse_process_notes (XEXP (x, i), object, changed), 0); + cse_process_notes (XEXP (x, i), object, &did_change), 0); + + /* We may need to rebuild the expression after substitution. + e.g. if the first operand of a PLUS is replaced by a constant. */ + if (did_change) + { + rtx simplified; + *changed = true; + simplified = simplify_rtx(x); + if (simplified) + x = simplified; + } return x; }