This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix PR optimization/12085


Hi Eric,

On Sun, 7 Dec 2003, Eric Botcazou wrote:
> > This is OK for mainline with one change.  I think that you should remove
> > the aborts inside the "#ifdef ENABLE_CHECKING" and always just return
> > NULL_TREE.  There's no point ICEing the compiler in the middle-end just
> > because the user has entered some dubious code.  Especially, when there's
> > a safe recovery strategy, i.e. just don't inline the function.
>
> Yes, but I'm not sure that my checks are not too tight.  Objective-C does
> really weird things, like calling a function which returns a struct through
> a prototype which returns an int (the underlying mode is the same though, in
> this case SI).  And I can think of a (pathological again) situation where
> they could be: if the function is casted to a type with a different non-void
> return value, and the return value is not used; in this case, I think
> inlining would be successful, while my checks will fail.

As you've pointed out, its exceptionally rare that function call type
checking is abused in this way.  You've argued that your testing has
revealed not a single example in a complete GCC bootstrap and regression
test run.  And even in these rare cases, not inlining in a function call
although a minor pessimization is certainly not the end of the world.
Inlining at the best of times is a hint, and if its a choice between
between inlining or not, vs. correctly inlining, correctness obviously
wins.

In your example, casting a function's return type to void, may be safe
in some circumstances, but consider the effects of a stack frame that
a caller has to clean up, or where the register used to hold the return
value needs to be considered clobbered, or whether we end up calling the
destructors of any temporary object returned from a function, or whether
we're passing in an appropriate "hidden" pointer for C++'s return value
optimization.  In many ABIs the existance of a return type significantly
affects the call sequence.  If it's even potentially unsafe, lets just
not inline the function call.

If you're worried about being too strict, you might also compare your
implementation against my similar patch to defend GCC's builtins from
similar abuse: http://gcc.gnu.org/ml/gcc-patches/2003-09/msg00771.html
[Note this even mentions PR optimization/12085].  Rather than compare
the types' modes this just ensures that integral values match integral
values, real values match real values, etc...  This is sufficient to
catch the double->int conversion in your testcase.  Builtins form a
significantly easier problem because they use only a limited set of
types.  We don't have to worry about coverting a struct of one type
into a struct of another, even if both have mode BLKmode, etc...


> The abort() is primarily here to detect such pessimizations, instead of
> silently preventing inlining.  And it will not occur in released compilers
> anyway.

I still believe "abort" should be reserved for situations where the
compiler itself has screwed up.  When the user screws up, ... well thats
just to be expected.  We could write a diagnostic to a log file explaining
why we chose not to inline (if we don't already).

Getting an ICE with --enable-checking but not without, should really
reflect a major problem with the compiler itself, not just a missing
-pedantic warning message (or some such).



> I'm really not prepared to put it on the 3.3 branch.  I'm not even
> sure it is worth while to put it on mainline at this point.


Again this is your call.  I don't want to force you to support a
patch you don't want to.  However, I will state that I'm happy to
stand behind your patch myself should we subsequently encounter any
problems with it.  I'd already put investigating the patch's possible
interactions with "always inline" on my TODO list.


The letter of the law is that both 3.3.3 and mainline are in
approximately the same state at the moment.  "Bug fixes only".
No new features or enhancements, even tiny non-intrusive ones.
Even code clean-ups are not allowed, with the one exception of
comments!  But my understanding is that at the moment, it doesn't
matter whether they are regressions or not.  Intrusiveness is
an issue, but in this case the change is within a single function.

If all the outstanding bugs PRs could be fixed by the end of December,
there is currently no political reason why both 3.3.3 and 3.4 shouldn't
be completely bug free (to the best of our knowledge) at the point they
ship.  Once 3.4 branches or the release manager freezes 3.3.3 in the
new year, we then have to make compromises.  From the middle-end
and optimizer perspective, we've fixed more 3.3.3 targeted bugs in
the last two weeks than there are currently outstanding.  And all
without destablizing the tree.  There hasn't been a new bootstrap
failure (even on lesser platforms) or patch that's needed reverting on
mainline or 3.3 for quite sometime.  Infact I've been surprised how
non-intrusive and safe these bug-fixes have been, many "obviously"
correct.


In the case of your patch, even if we disabled all inlining of
functions called through casts, I doubt that anyone would notice.
It would be interesting to compare the support for this in other
compilers.  But given that we currently silently generate incorrect
code for most of these cases, we're doing well supporting the forms
of type coercion that you're proposing.

Would you feel happier with the much simpler variant for 3.3?

+  if (TREE_CODE (TREE_OPERAND (t, 0)) == NOP_EXPR)
+    return NULL_TREE;



If its not clear from the subtext above, its difficult to express
in words how much I appreciate your many recent contributions.
I won't/can't say anything against anyone who even in stage3 chooses
not to help resolve PRs against mainline or the release branch, but
what I can do is to express my gratitude towards those who do.

[But don't think I won't revert a patch, if you screw up! :>]

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]