Re: GCC 3.1 Issues

[Daniel Berlin <dan at dberlin dot org>]

On Sun, 2002-03-10 at 08:29, Roger Sayle wrote:
> 
> Hi Andreas,
> > I'm a little bit puzzled.
> > But the inline function does not "modify memory in an unpredictable
> > fashion", we just access it.
> >
> > So, my questions are:
> > - is the inline function correct?
> > - is the manual correct?
> 
> That was my first take on the problem which was why I submitted the
> patch.  However, Richard's suggestion of adding a "memory" clobber
> does indeed fix the problem (both with your original __asm__ and my
> distilled test case).
> 
> If rth's interpretation is right, the manual needs to be corrected.
> If the manual is right, someone should consider applying my patch.
> In the worst case my patch only disables "store motion" across __asm__
> statements, rather than disabling them entirely.
> 
> 
> As the SPEC2000 guy :>, could you check how much store motion actually
> effects performance?  Seems silly to argue over a possibly unmeasurable
> optimization.

Um, right now?
It won't make a measurable difference, since it'll never think stores
are okay when they are.
It also won't hoist anything loop already doesn't handle.

> 
> 
> > There were discussions on the list (some referenced via PR 5200) that
> > mentioned that store motion is broken.  I'm more than happy if
> > somebody investigates those claims,
> 
> The messages referenced via PR 5200 refer to a single list of potential
> problems in http://gcc.gnu.org/ml/gcc-patches/2001-11/msg00777.html by
> Dan Berlin.  However it doesn't cite a single reproduceable testcase
> or application to check that these concerns haven't already been fixed.
That's because nobody cared to fix it.
I have provided more information, and patches, to the few people who
emailed me privately wanting to fix it.

> 
> To reply to that mails list of concerns:
> [1] store_ops_ok might still need to be renamed store_ops_not_ok, but
>     provided it is used corrrectly this is not a bug.
Errr, it's not used correctly. That's the whole problem.
Thus, store motion never does anything right now that could be right.

> [2] Was hopefully fixed by Jakub's gcse.c patch on 2001-12-05.
Probably.

> [3] load_kills_store calls true_dependence in alias.c which should be
>     able to handle alias sets, just fine.
This is not the problem.
The problem is that it never checks if the load kills the store, because
it thinks it has no reason to.

> 
> I'll check if disabling store motion helps anything in the test-suite,
> as suggested by http://gcc.gnu.org/ml/gcc-patches/2001-08/msg00268.html,
> but if nothing changes, I'd be inclined to trust 3.1's application testing
> and the nearly 18000 tests in gcc's testsuite.
> 
Change the value of the store_ops_ok test such that store motion does
something sometimes, and you'll start breaking a few tests on some
platforms.
Enabling it is just a waste of time anyway right now.
It does nothing loop doesn't do, and what it does do, it doesn't do
right.
> The best person to ask to clarify the situation is Dan Berlin.  Dan,
> do you still believe that store-motion is broken?  Can you provide a
> testcase to substantiate your claims?

I haven't tried in months, since nothing in store motion has changed.
I have a patch to improve/fix most of store motion, but it still failed
bootstrap java on x86 due to a single mismoved store because of aliasing
bugs at the time (it worked fine and passed make check on ppc and
sparc).
Maybe it works now that aliasing has changed significantly.


> 
> Roger
> --
> Roger Sayle,                         E-mail: roger@eyesopen.com
> OpenEye Scientific Software,         WWW: http://www.eyesopen.com/
> Suite 1107, 3600 Cerrillos Road,     Tel: (+1) 505-473-7385
> Santa Fe, New Mexico, 87507.         Fax: (+1) 505-473-0833