Patchwork [rs6000] disable mfcr pattern for TARGET_ISEL

login
register
mail settings
Submitter Nathan Froyd
Date July 20, 2010, 8:45 p.m.
Message ID <20100720204506.GN26037@codesourcery.com>
Download mbox | patch
Permalink /patch/59371/
State New
Headers show

Comments

Nathan Froyd - July 20, 2010, 8:45 p.m.
mfcr can be a slow instruction, particular for multiple-issue
processors, where it requires serializing the instruction stream to
ensure that CR has been properly updated.  The primary pattern where
mfcr occurs deals with moving an individual bit from CR to a GPR.  This
pattern not only uses mfcr, but also binds the mfcr and accompanying
rlwinm together instead of permitting them to be scheduled separately.

On TARGET_ISEL processors, however, we can improve the matter somewhat
by using isel instead of mfcr.  With isel, there's no need to
synchronize on CR as a whole, just an individual field.  The
instructions that load zero and one can also be scheduled separately
from the isel itself.  The patch below enables this improvement by
turning off the aforementioned mfcr/rlwinm pattern for TARGET_ISEL
processors.

The patch could be improved; there's a TARGET_POWERPC64 pattern that's
identical save for the modes of registers slightly further on in the
file.  I don't have a power7 system to test the equivalent 64-bit path
on, but if somebody was willing to test on such a system for me, I could
tweak the patch accordingly.  (Such a tweaked patch could also use mode
iterators, which would be an improvement.)

Tested with cross to powerpc-eabispe.  OK to commit?

-Nathan

	* config/rs6000/rs6000.md (define_insn ""): Enable only for
	!TARGET_ISEL targets.
David Edelsohn - July 21, 2010, 3:32 p.m.
On Tue, Jul 20, 2010 at 4:45 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> mfcr can be a slow instruction, particular for multiple-issue
> processors, where it requires serializing the instruction stream to
> ensure that CR has been properly updated.  The primary pattern where
> mfcr occurs deals with moving an individual bit from CR to a GPR.  This
> pattern not only uses mfcr, but also binds the mfcr and accompanying
> rlwinm together instead of permitting them to be scheduled separately.
>
> On TARGET_ISEL processors, however, we can improve the matter somewhat
> by using isel instead of mfcr.  With isel, there's no need to
> synchronize on CR as a whole, just an individual field.  The
> instructions that load zero and one can also be scheduled separately
> from the isel itself.  The patch below enables this improvement by
> turning off the aforementioned mfcr/rlwinm pattern for TARGET_ISEL
> processors.
>
> The patch could be improved; there's a TARGET_POWERPC64 pattern that's
> identical save for the modes of registers slightly further on in the
> file.  I don't have a power7 system to test the equivalent 64-bit path
> on, but if somebody was willing to test on such a system for me, I could
> tweak the patch accordingly.  (Such a tweaked patch could also use mode
> iterators, which would be an improvement.)
>
> Tested with cross to powerpc-eabispe.  OK to commit?
>
> -Nathan
>
>        * config/rs6000/rs6000.md (define_insn ""): Enable only for
>        !TARGET_ISEL targets.

First, the ChangeLog needs a better identifier.  define_insn "" ?  At
least add a "*" name to the unnamed pattern.

Do processors with TARGET_ISEL also implement MFCRF?  mfcrf does not
have the same expense as mfcr.  There may be other reasons to use ISEL
more aggressively, but I am surprised that this would be a problem on
embedded processors with ISEL.  Is this for old processors with ISEL
or did new processors not enable TARGET_MFCRF in GCC?

Thanks, David
Nathan Froyd - July 21, 2010, 3:51 p.m.
On Wed, Jul 21, 2010 at 11:32:03AM -0400, David Edelsohn wrote:
> On Tue, Jul 20, 2010 at 4:45 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On TARGET_ISEL processors, however, we can improve the matter somewhat
> > by using isel instead of mfcr.  With isel, there's no need to
> > synchronize on CR as a whole, just an individual field.  The
> > instructions that load zero and one can also be scheduled separately
> > from the isel itself.  The patch below enables this improvement by
> > turning off the aforementioned mfcr/rlwinm pattern for TARGET_ISEL
> > processors.
> >
> >        * config/rs6000/rs6000.md (define_insn ""): Enable only for
> >        !TARGET_ISEL targets.
> 
> First, the ChangeLog needs a better identifier.  define_insn "" ?  At
> least add a "*" name to the unnamed pattern.

:)  Fair enough.

> Do processors with TARGET_ISEL also implement MFCRF?  mfcrf does not
> have the same expense as mfcr.  There may be other reasons to use ISEL
> more aggressively, but I am surprised that this would be a problem on
> embedded processors with ISEL.  Is this for old processors with ISEL
> or did new processors not enable TARGET_MFCRF in GCC?

The motivation was for some newer embedded processors.  Older processors
with ISEL do not implement MFCRF; they can still benefit from using ISEL
instead of MFCR.  Some newer embedded processors do implement MFCRF, but
MFCRF on those implementations carries the same synchronization
penalties as MFCR, so it's beneficial to use ISEL there as well.

I see now that the pattern cleverly uses 'Q' so that it works for
TARGET_MFCRF as well.  Will !TARGET_ISEL || TARGET_MFCRF DTRT?

-Nathan
David Edelsohn - July 21, 2010, 4:34 p.m.
On Wed, Jul 21, 2010 at 11:51 AM, Nathan Froyd <froydnj@codesourcery.com> wrote:

>> Do processors with TARGET_ISEL also implement MFCRF?  mfcrf does not
>> have the same expense as mfcr.  There may be other reasons to use ISEL
>> more aggressively, but I am surprised that this would be a problem on
>> embedded processors with ISEL.  Is this for old processors with ISEL
>> or did new processors not enable TARGET_MFCRF in GCC?
>
> The motivation was for some newer embedded processors.  Older processors
> with ISEL do not implement MFCRF; they can still benefit from using ISEL
> instead of MFCR.  Some newer embedded processors do implement MFCRF, but
> MFCRF on those implementations carries the same synchronization
> penalties as MFCR, so it's beneficial to use ISEL there as well.

Someone implemented mfcrf as inefficiently as mfcr?  Sigh.

> I see now that the pattern cleverly uses 'Q' so that it works for
> TARGET_MFCRF as well.  Will !TARGET_ISEL || TARGET_MFCRF DTRT?

I'm not sure what you are trying to capture with the new final
constraint, unless you now are saying that the pattern *is* a good
idea for TARGET_ISEL processors with TARGET_MFCRF, which sort of
contradicts your statement above.

Thanks, David
Nathan Froyd - July 21, 2010, 4:48 p.m.
On Wed, Jul 21, 2010 at 12:34:17PM -0400, David Edelsohn wrote:
> On Wed, Jul 21, 2010 at 11:51 AM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > The motivation was for some newer embedded processors.  Older processors
> > with ISEL do not implement MFCRF; they can still benefit from using ISEL
> > instead of MFCR.  Some newer embedded processors do implement MFCRF, but
> > MFCRF on those implementations carries the same synchronization
> > penalties as MFCR, so it's beneficial to use ISEL there as well.
> 
> Someone implemented mfcrf as inefficiently as mfcr?  Sigh.
> 
> > I see now that the pattern cleverly uses 'Q' so that it works for
> > TARGET_MFCRF as well.  Will !TARGET_ISEL || TARGET_MFCRF DTRT?
> 
> I'm not sure what you are trying to capture with the new final
> constraint, unless you now are saying that the pattern *is* a good
> idea for TARGET_ISEL processors with TARGET_MFCRF, which sort of
> contradicts your statement above.

Whoops, yes.

Is there a way we can move this forward?

-Nathan
Nathan Froyd - July 26, 2010, 4:43 p.m.
On Mon, Jul 26, 2010 at 11:41:09AM -0500, Edmar Wienskoski wrote:
> The original patch breaks the bootstrap for e500v2.
> A cross build like you did, do not trigger this:
>
> ../../../src_gcc/libgcc/../gcc/libgcc2.c: In function ‘__mulsc3’:
> ../../../src_gcc/libgcc/../gcc/libgcc2.c:1885:1: error: unrecognizable insn:
> (insn 46 45 47 5 ../../../src_gcc/libgcc/../gcc/libgcc2.c:1844 (set (reg:SI
> 263)
>         (eq:SI (reg:CCEQ 265)
>             (const_int 0 [0]))) -1 (nil))
> ../../../src_gcc/libgcc/../gcc/libgcc2.c:1885:1: internal compiler error: in
> extract_insn, at recog.c:2127

I think I saw this once I changed powerpc-eabispe to generate code for
e500 by default.  Back to the drawing board.

-Nathan

Patch

Index: config/rs6000/rs6000.md
===================================================================
--- config/rs6000/rs6000.md	(revision 162348)
+++ config/rs6000/rs6000.md	(working copy)
@@ -13052,7 +13052,7 @@  (define_insn ""
 	(match_operator:SI 1 "scc_comparison_operator"
 			   [(match_operand 2 "cc_reg_operand" "y")
 			    (const_int 0)]))]
-  ""
+  "!TARGET_ISEL"
   "mfcr %0%Q2\;{rlinm|rlwinm} %0,%0,%J1,1"
   [(set (attr "type")
      (cond [(ne (symbol_ref "TARGET_MFCRF") (const_int 0))