* [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL
@ 2010-07-20 20:45 Nathan Froyd
2010-07-21 15:32 ` David Edelsohn
0 siblings, 1 reply; 6+ messages in thread
From: Nathan Froyd @ 2010-07-20 20:45 UTC (permalink / raw)
To: gcc-patches; +Cc: dje.gcc
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.
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))
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL
2010-07-20 20:45 [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL Nathan Froyd
@ 2010-07-21 15:32 ` David Edelsohn
2010-07-21 15:51 ` Nathan Froyd
0 siblings, 1 reply; 6+ messages in thread
From: David Edelsohn @ 2010-07-21 15:32 UTC (permalink / raw)
To: Nathan Froyd; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL
2010-07-21 15:32 ` David Edelsohn
@ 2010-07-21 15:51 ` Nathan Froyd
2010-07-21 16:34 ` David Edelsohn
0 siblings, 1 reply; 6+ messages in thread
From: Nathan Froyd @ 2010-07-21 15:51 UTC (permalink / raw)
To: David Edelsohn; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL
2010-07-21 15:51 ` Nathan Froyd
@ 2010-07-21 16:34 ` David Edelsohn
2010-07-21 16:49 ` Nathan Froyd
0 siblings, 1 reply; 6+ messages in thread
From: David Edelsohn @ 2010-07-21 16:34 UTC (permalink / raw)
To: Nathan Froyd; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL
2010-07-21 16:34 ` David Edelsohn
@ 2010-07-21 16:49 ` Nathan Froyd
[not found] ` <AANLkTikqRe1aGizRF4tJhL+roXnQzWa8bu1B2X44ERra@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Nathan Froyd @ 2010-07-21 16:49 UTC (permalink / raw)
To: David Edelsohn; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-26 16:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-20 20:45 [PATCH,rs6000] disable mfcr pattern for TARGET_ISEL Nathan Froyd
2010-07-21 15:32 ` David Edelsohn
2010-07-21 15:51 ` Nathan Froyd
2010-07-21 16:34 ` David Edelsohn
2010-07-21 16:49 ` Nathan Froyd
[not found] ` <AANLkTikqRe1aGizRF4tJhL+roXnQzWa8bu1B2X44ERra@mail.gmail.com>
2010-07-26 17:14 ` Nathan Froyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).