This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix *anddi_2 (PR target/59101)
- From: Uros Bizjak <ubizjak at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Richard Henderson <rth at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 13 Nov 2013 20:12:41 +0100
- Subject: Re: [PATCH] Fix *anddi_2 (PR target/59101)
- Authentication-results: sourceware.org; auth=none
- References: <20131113170358 dot GS27813 at tucnak dot zalov dot cz> <CAFULd4Y_eSBEbf9KzCpOvw4a-VhwcGV4i2W4MYWoKs3Uf6cPCA at mail dot gmail dot com> <20131113181247 dot GV27813 at tucnak dot zalov dot cz>
On Wed, Nov 13, 2013 at 7:12 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Nov 13, 2013 at 06:38:00PM +0100, Uros Bizjak wrote:
>> > --- gcc/config/i386/i386.md.jj 2013-11-12 11:31:31.000000000 +0100
>> > +++ gcc/config/i386/i386.md 2013-11-13 10:14:10.981609589 +0100
>> > @@ -7978,7 +7978,12 @@ (define_insn "*anddi_2"
>> > (const_int 0)))
>> > (set (match_operand:DI 0 "nonimmediate_operand" "=r,r,rm")
>> > (and:DI (match_dup 1) (match_dup 2)))]
>> > - "TARGET_64BIT && ix86_match_ccmode (insn, CCNOmode)
>> > + "TARGET_64BIT
>> > + && ix86_match_ccmode (insn, CONST_INT_P (operands[2])
>> > + && INTVAL (operands[2]) > 0
>> > + && (INTVAL (operands[2])
>> > + & (HOST_WIDE_INT_1 << 31)) != 0
>> > + ? CCZmode : CCNOmode)
>>
>> "Z" constraint, a.k.a x86_64_zext_immediate_operand won't allow
>> constants with high bits set.
>
> But x86_64_immediate_operand will.
Yes. I haven't notice "e" in operands[2].
>> It looks to me, we can use mode_signbit_p here, and simplify the expression to
>
> No, because mode_signbit_p tests if the constant is equal to the
> sign bit, while we need to test whether the sign bit is set.
> For & 0x80000000 it is the same thing, but for & 0x80000001 it is not.
>
>> ix86_match_ccmode (insn, mode_signbit_p (SImode, operands[2])
>> ? CCZmode : CCNOmode)
>
> Even if mode_signbit_p did something different, that would pessimize:
> long long
> foo (long long a)
> {
> long long b = a & 0xfffffffff0000000LL;
> if (b > 0)
> bar ();
> return b;
Eh, it should read val_signbit_known_set_p.
> So what about this version? If x86_64_zext_immediate_operand
> returns true for non-CONST_INT (CONST_DOUBLE won't happen for DImode
> operand with 64-bit HWI), it might be SYMBOL_REF/LABEL_REF etc. and
> it is IMHO better to just assume it might have bit 31 set.
Yes, this is correct.
>
> 2013-11-13 Jakub Jelinek <jakub@redhat.com>
>
> PR target/59101
> * config/i386/i386.md (*anddi_2): Only allow CCZmode if
> operands[2] is x86_64_zext_immediate_operand that might
> have bit 31 set.
>
> * gcc.c-torture/execute/pr59101.c: New test.
>
> --- gcc/config/i386/i386.md.jj 2013-11-13 18:32:48.586808734 +0100
> +++ gcc/config/i386/i386.md 2013-11-13 19:07:05.648122323 +0100
> @@ -7978,7 +7978,20 @@ (define_insn "*anddi_2"
> (const_int 0)))
> (set (match_operand:DI 0 "nonimmediate_operand" "=r,r,rm")
> (and:DI (match_dup 1) (match_dup 2)))]
> - "TARGET_64BIT && ix86_match_ccmode (insn, CCNOmode)
> + "TARGET_64BIT
> + && ix86_match_ccmode (insn,
> + /* If we are going to emit andl instead of andq,
> + and the operands[2] constant might have the
> + SImode sign bit set, make sure the sign flag isn't
> + tested, because the instruction will set the sign
> + flag based on bit 31 rather than bit 63.
> + If it isn't CONST_INT, conservatively assume
> + it might have bit 31 set. */
> + (x86_64_zext_immediate_operand (operands[2], VOIDmode)
> + && (!CONST_INT_P (operands[2])
> + || (INTVAL (operands[2])
> + & (HOST_WIDE_INT_1 << 31)) != 0))
> + ? CCZmode : CCNOmode)
I'd suggest adding following condition (together with your comment),
otherwise equivalent to the condition above, but IMO much more
descriptive:
Index: i386.md
===================================================================
--- i386.md (revision 204750)
+++ i386.md (working copy)
@@ -7978,7 +7978,12 @@
(const_int 0)))
(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,rm")
(and:DI (match_dup 1) (match_dup 2)))]
- "TARGET_64BIT && ix86_match_ccmode (insn, CCNOmode)
+ "TARGET_64BIT
+ && ix86_match_ccmode
+ (insn, (satisfies_constraint_Z (operands[2])
+ && (!CONST_INT_P (operands[2])
+ || val_signbit_known_set_p (SImode, INTVAL
(operands[2]))))
+ ? CCZmode : CCNOmode)
&& ix86_binary_operator_ok (AND, DImode, operands)"
"@
and{l}\t{%k2, %k0|%k0, %k2}
But I'll leave the decision to you.
Patch is OK everywhere, with or without the suggested change.
Thanks,
Uros.