Fw: [patch] conditional store elimination

[Tehila Meyzels <TEHILA at il dot ibm dot com>]

----- Forwarded by Tehila Meyzels/Haifa/IBM on 01/11/2007 22:17 -----

Tehila Meyzels/Haifa/IBM wrote on 29/10/2007 22:03:31:

> ----- Forwarded by Tehila Meyzels/Haifa/IBM on 29/10/2007 22:00 -----
>
> Tehila Meyzels/Haifa/IBM wrote on 11/10/2007 12:04:41:
>
> > Hi,
> >
> > On 8/28/07, Michael Matz <matz@suse.de> wrote:
> >
> > >Hi,
> > >
> > >this patch fixes PR middle-end/27313 and thereby a performance problem
on
> > >456.hmmer (which now runs in 798 instead of 1080 seconds, i.e. 26%
> > >improvement).  We talked about how to integrate this better with if
> > >conversion and make it also do load hoisting, but I fear we won't get
to
> > >that during the time left for stage 2, and meanwhile I'd really like
to
> > >have that improvement in 4.3.
> > >
> > >I tweaked the patch a bit to only do the transformation by default if
the
> > >target has conditional moves.
> >
> > Sorry, but I didn't find where in the patch you are checking this.
> > Could you please enlighten?
> >
> > >  Additionally it now is applied only when
> > >the store in question is non-trapping because of another shadowing mem

> > >access, which then also means that the value is in cache already (i.e.
I
> > >don't do it when only TREE_NOTRAP is set).
> > >
> > >For reference again, here is what the transformation does:
> > >
> > >   ... some access to *p ...
> > >   if (cond)
> > >     *p = X;
> > >
> > >--->
> > >
> > >   cstore = *p;
> > >   if (cond)
> > >     cstore = X;
> > >   *p = cstore;
> > >
> >
> > There is another option, that will prevent the need of speculative load
> > (and hence all the trapping/non trapping check):
> > You can transform:
> >
> >    if (cond)
> >      *p = X;
> >
> > ---------->
> >
> >  q (points to a dummy stack slot)
> >  if (cond)
> >    q=p;
> >  *q = X;
> >
> > i.e., choose the address (that you store to) instead of the value.

I would like to re-suggest this. It will prevent all the trapping/non-
trapping problems...
(And, thanks to Ayal Zaks for the creative idea...)

> >
> > Few other points...
> > What do you think about extending this to handle few conditional
> stores (and
> > not only one)?
> > What is your opinion that also conditional load hoisting will be
> > included here?
> >
> > >Later passes then have the possibility to transform the if into a
> > >conditional store (and transforming the *p read to a copy).
> > >
> > >Bootstrapped and tested (all langs except ada) on x86_64-linux.
> >
> > FYI, I intend to check it also on SPU. Will keep you posted... :-)
>
> Just a small update...
> It is (indeed) working on SPU.

Well... I looked on it more closer today.
It seems to me, that cselim pass doesn't do exactly what you wrote.
Attached are a testcase I wrote + dump before the cselim pass + dump
after cselim pass.
(See attached file: before_cselim.txt)(See attached file: cselim_test.txt)
(See attached file: after_cselim.txt)

It looks like the pass adds a conditional load (in the else block),
and this is not what I expected (according to your documentation,
the speculative store suppose to be before the condition
(i.e., unconditional).
More than that, since there is no conditional load hoisting yet
(I intend to implement this transformation), the if-conversion can't
work afterwards (and indeed, on SPU it doesn't...).
Maybe this is another reason why conditional load hoisting should
be included in this pass as well.

One more thing, I realized that in case there is an else block that
includes only a 'goto', the cselim transformation doesn't work.
(i.e., a 'diamond' if-then-else instead of a 'triangle' if-then-else)
This 2 cases ("triangle" if-then-else and "diamond" if-then-else with
only 'goto' in the else BB) are pretty similar, so I think this is
something cselim should handle as well.

Regards,
Tehila.
>
> Tehila.
>
> >
> > Tehila.
> > >
> > >
> > >Ciao,
> > >Michael.

Attachment: before_cselim.txt
Description: Text document

Attachment: cselim_test.txt
Description: Text document

Attachment: after_cselim.txt
Description: Text document