Re: [PATCH] Fix CSE bogus RTL creation

[Mark Mitchell <mark@codesourcery.com>]

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.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713