| Message ID | 20100720204506.GN26037@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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))