| Submitter | Uros Bizjak |
|---|---|
| Date | April 30, 2012, 5:14 p.m. |
| Message ID | <CAFULd4ZEe1WW2cgd7_ENoF-BviuOH65ArX1nFqP9e0SyxiQs3w@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/155926/ |
| State | New |
| Headers | show |
Comments
Resending in plain text mode so it goes through. Teresa On Mon, Jul 23, 2012 at 12:03 PM, Teresa Johnson <tejohnson@google.com> wrote: > Any possibility of getting these patches (186979 and 186993), along with > r184891 (which added the and->zext splitter), backported to the 4_7 branch? > I found a performance issue where "andw $0xff, %reg" was not being converted > to movzbl when the source and target registers are the same, resulting in > LCP stalls, which is fixed by this series of patches. > > Thanks, > Teresa > > > On Mon, Apr 30, 2012 at 10:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> On Mon, Apr 30, 2012 at 3:10 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> > On Mon, Apr 30, 2012 at 02:54:05PM +0200, Uros Bizjak wrote: >> >> > My recent changes to zero_extend expanders should handle this >> >> > automatically, and will undo generation of zero_extend pattern. >> >> > Please >> >> > see zero_extend<mode>si2_and expander, and how it handles >> >> > TARGET_ZERO_EXTEND_WITH_AND targets. >> >> >> >> Attached patch implements this idea. In addition, it fixes the >> >> splitter to not change output mode of zero_extension from HImode and >> >> QImode from DImode to SImode. Although they generate the same >> >> instruction, I think we should better keep original mode here. >> > >> > Thanks. I was trying this morning slightly different patch for the >> > same, >> > but strangely it failed bootstrap, and didn't get around to analysing >> > why a mem store had (zero_extend (subreg (reg))) on a RHS. >> > >> >> + operands[1] = gen_lowpart (mode, operands[1]); >> >> + >> >> + if (GET_MODE (operands[0]) == DImode) >> >> + insn = (mode == SImode) >> >> + ? gen_zero_extendsidi2 >> >> + : (mode == HImode) >> >> + ? gen_zero_extendhidi2 >> >> + : gen_zero_extendqidi2; >> >> + else if (GET_MODE (operands[0]) == SImode) >> >> + insn = (mode == HImode) >> >> + ? gen_zero_extendhisi2 >> >> + : gen_zero_extendqisi2; >> >> + else if (GET_MODE (operands[0]) == HImode) >> >> + insn = gen_zero_extendqihi2; >> >> else >> >> - ix86_expand_binary_operator (AND, <MODE>mode, operands); >> >> + gcc_unreachable (); >> >> + >> >> + emit_insn (insn (operands[0], operands[1])); >> > >> > IMHO you should use <MODE>mode instead of GET_MODE (operands[0]) >> > in all of the above, then the compiler can actually optimize >> > it at compile time. >> >> 2012-04-30 Uros Bizjak <ubizjak@gmail.com> >> >> * config/i386/i386.md (and<mode>3): Change runtime operand mode >> checks >> to compile-time "mode == <MODE>mode" checks. >> (and splitter): Ditto. >> >> Tested on x86_64-pc-linux-gnu, committed to mainline SVN. >> >> Uros. > > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >
On Mon, Jul 23, 2012 at 10:17 PM, Teresa Johnson <tejohnson@google.com> wrote: > Resending in plain text mode so it goes through. > Teresa > > On Mon, Jul 23, 2012 at 12:03 PM, Teresa Johnson <tejohnson@google.com> wrote: >> Any possibility of getting these patches (186979 and 186993), along with >> r184891 (which added the and->zext splitter), backported to the 4_7 branch? >> I found a performance issue where "andw $0xff, %reg" was not being converted >> to movzbl when the source and target registers are the same, resulting in >> LCP stalls, which is fixed by this series of patches. These looks too risky, especially r184891. I have waited with this patch after 4.7 branched just because of this risk factor. BTW: This part was wrong, there is no xmm->xmm movq insn: (*zero_extendsidi2_rex64): Add x,x alternative. (*zero_extendsidi2): Ditto. Uros.
On Tue, Jul 24, 2012 at 3:18 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > > On Mon, Jul 23, 2012 at 10:17 PM, Teresa Johnson <tejohnson@google.com> wrote: > > Resending in plain text mode so it goes through. > > Teresa > > > > On Mon, Jul 23, 2012 at 12:03 PM, Teresa Johnson <tejohnson@google.com> wrote: > >> Any possibility of getting these patches (186979 and 186993), along with > >> r184891 (which added the and->zext splitter), backported to the 4_7 branch? > >> I found a performance issue where "andw $0xff, %reg" was not being converted > >> to movzbl when the source and target registers are the same, resulting in > >> LCP stalls, which is fixed by this series of patches. > > These looks too risky, especially r184891. I have waited with this > patch after 4.7 branched just because of this risk factor. Ok, I will plan to port these directly to the google branches then. > > > BTW: This part was wrong, there is no xmm->xmm movq insn: > > (*zero_extendsidi2_rex64): Add x,x alternative. > (*zero_extendsidi2): Ditto. Yes, it looks like I need these 3: ------------------------------------------------------------------------ r188648 | uros | 2012-06-14 23:53:28 -0700 (Thu, 14 Jun 2012) | 3 lines Changed paths: M /trunk/gcc/ChangeLog M /trunk/gcc/config/i386/i386.md (*zero_extendsidi2_rex64): Remove isa attribute. ------------------------------------------------------------------------ r188634 | uros | 2012-06-14 12:38:12 -0700 (Thu, 14 Jun 2012) | 6 lines Changed paths: M /trunk/gcc/ChangeLog M /trunk/gcc/config/i386/i386.md Fix my previous commit to: * config/i386/i386.md (*zero_extendsidi2): Remove x,x alternative. (*zero_extendsidi2_rex64): Ditto. ------------------------------------------------------------------------ r188630 | uros | 2012-06-14 11:51:36 -0700 (Thu, 14 Jun 2012) | 5 lines Changed paths: M /trunk/gcc/ChangeLog M /trunk/gcc/config/i386/i386.md * config/i386/i386.md (*zero_extendsidi2): Mark movd alternatives SSE2 only. Remove x,x alternative. ------------------------------------------------------------------------ Thanks, Teresa > > Uros. -- Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Patch
Index: config/i386/i386.md =================================================================== --- config/i386/i386.md (revision 186992) +++ config/i386/i386.md (working copy) @@ -7695,7 +7695,7 @@ (match_operand:SWIM 2 "<general_szext_operand>")))] "" { - enum machine_mode mode = GET_MODE (operands[1]); + enum machine_mode mode = <MODE>mode; rtx (*insn) (rtx, rtx); if (CONST_INT_P (operands[2]) && REG_P (operands[0])) @@ -7710,30 +7710,28 @@ mode = QImode; } - if (mode == GET_MODE (operands[1])) + if (mode == <MODE>mode) { ix86_expand_binary_operator (AND, <MODE>mode, operands); DONE; } - operands[1] = gen_lowpart (mode, operands[1]); - - if (GET_MODE (operands[0]) == DImode) + if (<MODE>mode == DImode) insn = (mode == SImode) ? gen_zero_extendsidi2 : (mode == HImode) ? gen_zero_extendhidi2 : gen_zero_extendqidi2; - else if (GET_MODE (operands[0]) == SImode) + else if (<MODE>mode == SImode) insn = (mode == HImode) ? gen_zero_extendhisi2 : gen_zero_extendqisi2; - else if (GET_MODE (operands[0]) == HImode) + else if (<MODE>mode == HImode) insn = gen_zero_extendqihi2; else gcc_unreachable (); - emit_insn (insn (operands[0], operands[1])); + emit_insn (insn (operands[0], gen_lowpart (mode, operands[1]))); DONE; }) @@ -7884,9 +7882,7 @@ mode = QImode; } - operands[1] = gen_lowpart (mode, operands[1]); - - if (GET_MODE (operands[0]) == DImode) + if (<MODE>mode == DImode) insn = (mode == SImode) ? gen_zero_extendsidi2 : (mode == HImode) @@ -7894,14 +7890,15 @@ : gen_zero_extendqidi2; else { - /* Zero extend to SImode to avoid partial register stalls. */ - operands[0] = gen_lowpart (SImode, operands[0]); + if (<MODE>mode != SImode) + /* Zero extend to SImode to avoid partial register stalls. */ + operands[0] = gen_lowpart (SImode, operands[0]); insn = (mode == HImode) ? gen_zero_extendhisi2 : gen_zero_extendqisi2; } - emit_insn (insn (operands[0], operands[1])); + emit_insn (insn (operands[0], gen_lowpart (mode, operands[1]))); DONE; })