Patchwork Improve andq $0xffffffff, %reg handling (PR target/53110)

login
register
mail settings
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

Uros Bizjak - April 30, 2012, 5:14 p.m.
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 - July 23, 2012, 8:17 p.m.
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
>
Uros Bizjak - July 24, 2012, 10:18 a.m.
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.
Teresa Johnson - July 24, 2012, 1:34 p.m.
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;
 })