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

On Thu, Apr 26, 2012 at 5:34 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Apr 26, 2012 at 5:28 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
>>> > We have a splitter for reg1 = reg2 & 0xffffffff, but only if regnums
>>> > are different.  But movl %edi, %edi is a cheaper variant of
>>> > andq $0xffffffff, %rdi even with the same register and doesn't clobber
>>> > flags, so this patch attempts to expand it as a zero extension early.
>>> >
>>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>> >
>>> > 2012-04-25  Jakub Jelinek  <jakub@redhat.com>
>>> >
>>> >        PR target/53110
>>> >        * config/i386/i386.md (and<mode>3): For andq $0xffffffff, reg
>>> >        instead expand it as zero extension.
>>>
>>> Perhaps we can go all the way and also convert ANDs with $0xff and
>>> $0xffff to relevant zero_extend patterns, like in the referred
>>> splitter.
>>
>> I wasn't sure about 0xffff, on which CPUs it would be a win and on which it
>> would not.  0xffffffff is a win always.

It is always a win, especially when loading from memory. In the latter
case, no execution units are used, only load (memory read) unit.

> 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.

2012-04-30  Uros Bizjak  <ubizjak@gmail.com>

	* config/i386/i386.md (and<mode>3): Expand masking operations with
	0xff, 0xffff or 0xffffffff immediates to corresponding zero_extend RTX.
	(and splitter): Split to DImode zero_extend RTX for DImode operand[0].

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
{,-m32} and committed to mainline SVN.

Uros.

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.

	Jakub

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.

Maybe it was due to slightly wrong splitter? I was changing both, the
expander and the splitter in parallel, and didn't see any failure you
mentioned...

>> +  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.

You are right, I will change this in an incremental patch.

Thanks,
Uros.

On Mon, Apr 30, 2012 at 3:34 PM, 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.
>
> Maybe it was due to slightly wrong splitter? I was changing both, the
> expander and the splitter in parallel, and didn't see any failure you
> mentioned...

In fact, the problem was already in your original patch. There was a
check for REG_P (operands[1]), but it should check for REG_P
(operands[0]). There is no zero_extended store, only load.

Uros.