* [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)
@ 2018-05-02 7:42 Jakub Jelinek
2018-05-02 7:50 ` Uros Bizjak
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2018-05-02 7:42 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches
Hi!
operands[2] is an input operand in these define_insn_and_split patterns,
so when the masking needs to be still performed, we shouldn't modify that
input register; all the patterns are guarded with can_create_pseudo_p,
so we can create new pseudos there.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2018-05-02 Jakub Jelinek <jakub@redhat.com>
PR target/85582
* config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
*ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
*<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
clobber operands[2], instead use a new pseudo. Formatting fixes.
* gcc.c-torture/execute/pr85582-1.c: New test.
* gcc.c-torture/execute/pr85582-2.c: New test.
--- gcc/config/i386/i386.md.jj 2018-05-01 12:18:15.566832552 +0200
+++ gcc/config/i386/i386.md 2018-05-01 13:09:36.635164943 +0200
@@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
(match_operand:SI 2 "register_operand" "c")
(match_operand:SI 3 "const_int_operand")) 0)))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
+ "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -10385,8 +10385,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
- emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
+ if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ {
+ rtx tem = gen_reg_rtx (SImode);
+ emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
+ operands[2] = tem;
+ }
operands[2] = gen_lowpart (QImode, operands[2]);
@@ -10402,7 +10406,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
(match_operand:QI 2 "register_operand" "c")
(match_operand:QI 3 "const_int_operand"))))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
+ "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -10421,8 +10425,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
- emit_insn (gen_andqi3 (operands[2], operands[2], operands[3]));
+ if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ {
+ rtx tem = gen_reg_rtx (QImode);
+ emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
+ operands[2] = tem;
+ }
if (!rtx_equal_p (operands[6], operands[7]))
emit_move_insn (operands[6], operands[7]);
@@ -11118,7 +11126,7 @@ (define_insn_and_split "*<shift_insn><dw
(match_operand:SI 2 "register_operand" "c")
(match_operand:SI 3 "const_int_operand")) 0)))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
+ "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -11138,7 +11146,11 @@ (define_insn_and_split "*<shift_insn><dw
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
- emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
+ {
+ rtx tem = gen_reg_rtx (SImode);
+ emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
+ operands[2] = tem;
+ }
operands[2] = gen_lowpart (QImode, operands[2]);
@@ -11154,7 +11166,7 @@ (define_insn_and_split "*<shift_insn><dw
(match_operand:QI 2 "register_operand" "c")
(match_operand:QI 3 "const_int_operand"))))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
+ "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -11173,8 +11185,12 @@ (define_insn_and_split "*<shift_insn><dw
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
- emit_insn (gen_andqi3 (operands[2], operands[2], operands[3]));
+ if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ {
+ rtx tem = gen_reg_rtx (QImode);
+ emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
+ operands[2] = tem;
+ }
if (!rtx_equal_p (operands[4], operands[5]))
emit_move_insn (operands[4], operands[5]);
--- gcc/testsuite/gcc.c-torture/execute/pr85582-1.c.jj 2018-05-01 13:28:07.547933727 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr85582-1.c 2018-05-01 13:26:32.270868649 +0200
@@ -0,0 +1,21 @@
+/* PR target/85582 */
+
+int a, b, d = 2, e;
+long long c = 1;
+
+int
+main ()
+{
+ int g = 6;
+L1:
+ e = d;
+ if (a)
+ goto L1;
+ g--;
+ int i = c >> ~(~e | ~g);
+L2:
+ c = (b % c) * i;
+ if (!e)
+ goto L2;
+ return 0;
+}
--- gcc/testsuite/gcc.c-torture/execute/pr85582-2.c.jj 2018-05-01 13:28:10.332935636 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr85582-2.c 2018-05-01 13:28:00.615929000 +0200
@@ -0,0 +1,51 @@
+/* PR target/85582 */
+
+#ifdef __SIZEOF_INT128__
+typedef __int128 S;
+typedef unsigned __int128 U;
+#else
+typedef long long S;
+typedef unsigned long long U;
+#endif
+
+__attribute__((noipa)) S
+f1 (S x, int y)
+{
+ x = x << (y & 5);
+ x += y;
+ return x;
+}
+
+__attribute__((noipa)) S
+f2 (S x, int y)
+{
+ x = x >> (y & 5);
+ x += y;
+ return x;
+}
+
+__attribute__((noipa)) U
+f3 (U x, int y)
+{
+ x = x >> (y & 5);
+ x += y;
+ return x;
+}
+
+int
+main ()
+{
+ S a = (S) 1 << (sizeof (S) * __CHAR_BIT__ - 7);
+ S b = f1 (a, 12);
+ if (b != ((S) 1 << (sizeof (S) * __CHAR_BIT__ - 3)) + 12)
+ __builtin_abort ();
+ S c = (U) 1 << (sizeof (S) * __CHAR_BIT__ - 1);
+ S d = f2 (c, 12);
+ if ((U) d != ((U) 0x1f << (sizeof (S) * __CHAR_BIT__ - 5)) + 12)
+ __builtin_abort ();
+ U e = (U) 1 << (sizeof (U) * __CHAR_BIT__ - 1);
+ U f = f3 (c, 12);
+ if (f != ((U) 1 << (sizeof (U) * __CHAR_BIT__ - 5)) + 12)
+ __builtin_abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)
2018-05-02 7:42 [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582) Jakub Jelinek
@ 2018-05-02 7:50 ` Uros Bizjak
2018-05-02 8:02 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Uros Bizjak @ 2018-05-02 7:50 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, May 2, 2018 at 9:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> operands[2] is an input operand in these define_insn_and_split patterns,
> so when the masking needs to be still performed, we shouldn't modify that
> input register; all the patterns are guarded with can_create_pseudo_p,
> so we can create new pseudos there.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-05-02 Jakub Jelinek <jakub@redhat.com>
>
> PR target/85582
> * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
> *ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
> *<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
> clobber operands[2], instead use a new pseudo. Formatting fixes.
>
> * gcc.c-torture/execute/pr85582-1.c: New test.
> * gcc.c-torture/execute/pr85582-2.c: New test.
O.
Thanks,
Uros.
> --- gcc/config/i386/i386.md.jj 2018-05-01 12:18:15.566832552 +0200
> +++ gcc/config/i386/i386.md 2018-05-01 13:09:36.635164943 +0200
> @@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
> (match_operand:SI 2 "register_operand" "c")
> (match_operand:SI 3 "const_int_operand")) 0)))
> (clobber (reg:CC FLAGS_REG))]
> - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
> + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> && can_create_pseudo_p ()"
> "#"
> "&& 1"
> @@ -10385,8 +10385,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
>
> operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>
> - if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
> - emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
> + if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> + {
> + rtx tem = gen_reg_rtx (SImode);
> + emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
> + operands[2] = tem;
> + }
>
> operands[2] = gen_lowpart (QImode, operands[2]);
>
> @@ -10402,7 +10406,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
> (match_operand:QI 2 "register_operand" "c")
> (match_operand:QI 3 "const_int_operand"))))
> (clobber (reg:CC FLAGS_REG))]
> - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
> + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> && can_create_pseudo_p ()"
> "#"
> "&& 1"
> @@ -10421,8 +10425,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
>
> operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>
> - if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
> - emit_insn (gen_andqi3 (operands[2], operands[2], operands[3]));
> + if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> + {
> + rtx tem = gen_reg_rtx (QImode);
> + emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
> + operands[2] = tem;
> + }
>
> if (!rtx_equal_p (operands[6], operands[7]))
> emit_move_insn (operands[6], operands[7]);
> @@ -11118,7 +11126,7 @@ (define_insn_and_split "*<shift_insn><dw
> (match_operand:SI 2 "register_operand" "c")
> (match_operand:SI 3 "const_int_operand")) 0)))
> (clobber (reg:CC FLAGS_REG))]
> - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
> + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> && can_create_pseudo_p ()"
> "#"
> "&& 1"
> @@ -11138,7 +11146,11 @@ (define_insn_and_split "*<shift_insn><dw
> operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>
> if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
> - emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
> + {
> + rtx tem = gen_reg_rtx (SImode);
> + emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
> + operands[2] = tem;
> + }
>
> operands[2] = gen_lowpart (QImode, operands[2]);
>
> @@ -11154,7 +11166,7 @@ (define_insn_and_split "*<shift_insn><dw
> (match_operand:QI 2 "register_operand" "c")
> (match_operand:QI 3 "const_int_operand"))))
> (clobber (reg:CC FLAGS_REG))]
> - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
> + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> && can_create_pseudo_p ()"
> "#"
> "&& 1"
> @@ -11173,8 +11185,12 @@ (define_insn_and_split "*<shift_insn><dw
>
> operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>
> - if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
> - emit_insn (gen_andqi3 (operands[2], operands[2], operands[3]));
> + if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> + {
> + rtx tem = gen_reg_rtx (QImode);
> + emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
> + operands[2] = tem;
> + }
>
> if (!rtx_equal_p (operands[4], operands[5]))
> emit_move_insn (operands[4], operands[5]);
> --- gcc/testsuite/gcc.c-torture/execute/pr85582-1.c.jj 2018-05-01 13:28:07.547933727 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr85582-1.c 2018-05-01 13:26:32.270868649 +0200
> @@ -0,0 +1,21 @@
> +/* PR target/85582 */
> +
> +int a, b, d = 2, e;
> +long long c = 1;
> +
> +int
> +main ()
> +{
> + int g = 6;
> +L1:
> + e = d;
> + if (a)
> + goto L1;
> + g--;
> + int i = c >> ~(~e | ~g);
> +L2:
> + c = (b % c) * i;
> + if (!e)
> + goto L2;
> + return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr85582-2.c.jj 2018-05-01 13:28:10.332935636 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr85582-2.c 2018-05-01 13:28:00.615929000 +0200
> @@ -0,0 +1,51 @@
> +/* PR target/85582 */
> +
> +#ifdef __SIZEOF_INT128__
> +typedef __int128 S;
> +typedef unsigned __int128 U;
> +#else
> +typedef long long S;
> +typedef unsigned long long U;
> +#endif
> +
> +__attribute__((noipa)) S
> +f1 (S x, int y)
> +{
> + x = x << (y & 5);
> + x += y;
> + return x;
> +}
> +
> +__attribute__((noipa)) S
> +f2 (S x, int y)
> +{
> + x = x >> (y & 5);
> + x += y;
> + return x;
> +}
> +
> +__attribute__((noipa)) U
> +f3 (U x, int y)
> +{
> + x = x >> (y & 5);
> + x += y;
> + return x;
> +}
> +
> +int
> +main ()
> +{
> + S a = (S) 1 << (sizeof (S) * __CHAR_BIT__ - 7);
> + S b = f1 (a, 12);
> + if (b != ((S) 1 << (sizeof (S) * __CHAR_BIT__ - 3)) + 12)
> + __builtin_abort ();
> + S c = (U) 1 << (sizeof (S) * __CHAR_BIT__ - 1);
> + S d = f2 (c, 12);
> + if ((U) d != ((U) 0x1f << (sizeof (S) * __CHAR_BIT__ - 5)) + 12)
> + __builtin_abort ();
> + U e = (U) 1 << (sizeof (U) * __CHAR_BIT__ - 1);
> + U f = f3 (c, 12);
> + if (f != ((U) 1 << (sizeof (U) * __CHAR_BIT__ - 5)) + 12)
> + __builtin_abort ();
> + return 0;
> +}
>
> Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)
2018-05-02 7:50 ` Uros Bizjak
@ 2018-05-02 8:02 ` Jakub Jelinek
2018-05-02 8:09 ` Uros Bizjak
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2018-05-02 8:02 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches
On Wed, May 02, 2018 at 09:50:07AM +0200, Uros Bizjak wrote:
> > 2018-05-02 Jakub Jelinek <jakub@redhat.com>
> >
> > PR target/85582
> > * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
> > *ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
> > *<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
> > clobber operands[2], instead use a new pseudo. Formatting fixes.
> >
> > * gcc.c-torture/execute/pr85582-1.c: New test.
> > * gcc.c-torture/execute/pr85582-2.c: New test.
>
> O.
Thanks, committed. BTW, thinking about these some more,
isn't INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
incorrect? Say for <MODE_SIZE> being 4 this is INTVAL (operands[3]) <= 31
so it will accept & 0 to & 31 (that is correct), or e.g. & -2 (that is
incorrect).
Isn't the right guarding condition that
(INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
i.e. that we have guarantee the shift count doesn't have the topmost
relevant bit set and we can ignore bits above it (UB)?
And for the decision if we should use a masking or not perhaps
if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
!= (<MODE_SIZE> * BITS_PER_UNIT) - 1)
, again ignoring bits we don't care about.
> > --- gcc/config/i386/i386.md.jj 2018-05-01 12:18:15.566832552 +0200
> > +++ gcc/config/i386/i386.md 2018-05-01 13:09:36.635164943 +0200
> > @@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
> > (match_operand:SI 2 "register_operand" "c")
> > (match_operand:SI 3 "const_int_operand")) 0)))
> > (clobber (reg:CC FLAGS_REG))]
> > - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
> > + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> > && can_create_pseudo_p ()"
> > "#"
> > "&& 1"
> > @@ -10385,8 +10385,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
> >
> > operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
> >
> > - if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
> > - emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
> > + if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> > + {
> > + rtx tem = gen_reg_rtx (SImode);
> > + emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
> > + operands[2] = tem;
> > + }
> >
> > operands[2] = gen_lowpart (QImode, operands[2]);
> >
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)
2018-05-02 8:02 ` Jakub Jelinek
@ 2018-05-02 8:09 ` Uros Bizjak
2018-05-02 13:31 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Uros Bizjak @ 2018-05-02 8:09 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, May 2, 2018 at 10:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, May 02, 2018 at 09:50:07AM +0200, Uros Bizjak wrote:
>> > 2018-05-02 Jakub Jelinek <jakub@redhat.com>
>> >
>> > PR target/85582
>> > * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
>> > *ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
>> > *<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
>> > clobber operands[2], instead use a new pseudo. Formatting fixes.
>> >
>> > * gcc.c-torture/execute/pr85582-1.c: New test.
>> > * gcc.c-torture/execute/pr85582-2.c: New test.
>>
>> O.
>
> Thanks, committed. BTW, thinking about these some more,
> isn't INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> incorrect? Say for <MODE_SIZE> being 4 this is INTVAL (operands[3]) <= 31
> so it will accept & 0 to & 31 (that is correct), or e.g. & -2 (that is
> incorrect).
Hm, I wasn't thinking about undefined cases here.
> Isn't the right guarding condition that
> (INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
> i.e. that we have guarantee the shift count doesn't have the topmost
> relevant bit set and we can ignore bits above it (UB)?
Yes, based on above, it looks more robust, indeed.
> And for the decision if we should use a masking or not perhaps
> if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
> != (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> , again ignoring bits we don't care about.
Yes, looks more robust vs. UB, too (but updated guarding condition
won't allow UB values here). Let's also change this, for consistency.
The patch that changes the condition to your approach is pre-approved.
Thanks,
Uros.
>> > --- gcc/config/i386/i386.md.jj 2018-05-01 12:18:15.566832552 +0200
>> > +++ gcc/config/i386/i386.md 2018-05-01 13:09:36.635164943 +0200
>> > @@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
>> > (match_operand:SI 2 "register_operand" "c")
>> > (match_operand:SI 3 "const_int_operand")) 0)))
>> > (clobber (reg:CC FLAGS_REG))]
>> > - "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
>> > + "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
>> > && can_create_pseudo_p ()"
>> > "#"
>> > "&& 1"
>> > @@ -10385,8 +10385,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
>> >
>> > operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>> >
>> > - if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
>> > - emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
>> > + if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
>> > + {
>> > + rtx tem = gen_reg_rtx (SImode);
>> > + emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
>> > + operands[2] = tem;
>> > + }
>> >
>> > operands[2] = gen_lowpart (QImode, operands[2]);
>> >
>
> Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)
2018-05-02 8:09 ` Uros Bizjak
@ 2018-05-02 13:31 ` Jakub Jelinek
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Jelinek @ 2018-05-02 13:31 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches
On Wed, May 02, 2018 at 10:09:22AM +0200, Uros Bizjak wrote:
> On Wed, May 2, 2018 at 10:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, May 02, 2018 at 09:50:07AM +0200, Uros Bizjak wrote:
> >> > 2018-05-02 Jakub Jelinek <jakub@redhat.com>
> >> >
> >> > PR target/85582
> >> > * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
> >> > *ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
> >> > *<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
> >> > clobber operands[2], instead use a new pseudo. Formatting fixes.
> >> >
> >> > * gcc.c-torture/execute/pr85582-1.c: New test.
> >> > * gcc.c-torture/execute/pr85582-2.c: New test.
> >>
> >> O.
> >
> > Thanks, committed. BTW, thinking about these some more,
> > isn't INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> > incorrect? Say for <MODE_SIZE> being 4 this is INTVAL (operands[3]) <= 31
> > so it will accept & 0 to & 31 (that is correct), or e.g. & -2 (that is
> > incorrect).
>
> Hm, I wasn't thinking about undefined cases here.
It isn't just about undefined cases, see the testcase below which is
miscompiled because of these checks. Whether some bit in the mask is set or
not still doesn't imply anything on whether the other operand of the bit and
will have those bits set or clear.
> > Isn't the right guarding condition that
> > (INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
> > i.e. that we have guarantee the shift count doesn't have the topmost
> > relevant bit set and we can ignore bits above it (UB)?
>
> Yes, based on above, it looks more robust, indeed.
>
> > And for the decision if we should use a masking or not perhaps
> > if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
> > != (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> > , again ignoring bits we don't care about.
>
> Yes, looks more robust vs. UB, too (but updated guarding condition
> won't allow UB values here). Let's also change this, for consistency.
>
> The patch that changes the condition to your approach is pre-approved.
This is what I'll bootstrap/regtest tonight.
2018-05-03 Jakub Jelinek <jakub@redhat.com>
PR target/85582
* config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
*ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
*<shift_insn><dwi>3_doubleword_mask_1): In condition require that
the highest significant bit of the shift count mask is clear. In
check whether and[sq]i3 is needed verify that all significant bits
of the shift count other than the highest are set.
* gcc.c-torture/execute/pr85582-3.c: New test.
--- gcc/config/i386/i386.md.jj 2018-05-02 09:51:24.898661931 +0200
+++ gcc/config/i386/i386.md 2018-05-02 14:58:58.581494664 +0200
@@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
(match_operand:SI 2 "register_operand" "c")
(match_operand:SI 3 "const_int_operand")) 0)))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
+ "(INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -10385,7 +10385,8 @@ (define_insn_and_split "*ashl<dwi>3_doub
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
+ != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
{
rtx tem = gen_reg_rtx (SImode);
emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
@@ -10406,7 +10407,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
(match_operand:QI 2 "register_operand" "c")
(match_operand:QI 3 "const_int_operand"))))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
+ "(INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -10425,7 +10426,8 @@ (define_insn_and_split "*ashl<dwi>3_doub
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
+ != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
{
rtx tem = gen_reg_rtx (QImode);
emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
@@ -11126,7 +11128,7 @@ (define_insn_and_split "*<shift_insn><dw
(match_operand:SI 2 "register_operand" "c")
(match_operand:SI 3 "const_int_operand")) 0)))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
+ "(INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -11145,7 +11147,8 @@ (define_insn_and_split "*<shift_insn><dw
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
+ if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
+ != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
{
rtx tem = gen_reg_rtx (SImode);
emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
@@ -11166,7 +11169,7 @@ (define_insn_and_split "*<shift_insn><dw
(match_operand:QI 2 "register_operand" "c")
(match_operand:QI 3 "const_int_operand"))))
(clobber (reg:CC FLAGS_REG))]
- "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
+ "(INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
&& can_create_pseudo_p ()"
"#"
"&& 1"
@@ -11185,7 +11188,8 @@ (define_insn_and_split "*<shift_insn><dw
operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
- if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
+ if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
+ != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
{
rtx tem = gen_reg_rtx (QImode);
emit_insn (gen_andqi3 (tem, operands[2], operands[3]));
--- gcc/testsuite/gcc.c-torture/execute/pr85582-3.c.jj 2018-05-02 15:02:23.655654507 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr85582-3.c 2018-05-02 14:55:56.953353103 +0200
@@ -0,0 +1,55 @@
+/* PR target/85582 */
+
+#ifdef __SIZEOF_INT128__
+typedef __int128 S;
+typedef unsigned __int128 U;
+#else
+typedef long long S;
+typedef unsigned long long U;
+#endif
+
+__attribute__((noipa)) U
+f1 (U x, int y)
+{
+ return x << (y & -2);
+}
+
+__attribute__((noipa)) S
+f2 (S x, int y)
+{
+ return x >> (y & -2);
+}
+
+__attribute__((noipa)) U
+f3 (U x, int y)
+{
+ return x >> (y & -2);
+}
+
+int
+main ()
+{
+ U a = (U) 1 << (sizeof (U) * __CHAR_BIT__ - 7);
+ if (f1 (a, 5) != ((U) 1 << (sizeof (S) * __CHAR_BIT__ - 3)))
+ __builtin_abort ();
+ S b = (U) 0x101 << (sizeof (S) * __CHAR_BIT__ / 2 - 7);
+ if (f1 (b, sizeof (S) * __CHAR_BIT__ / 2) != (U) 0x101 << (sizeof (S) * __CHAR_BIT__ - 7))
+ __builtin_abort ();
+ if (f1 (b, sizeof (S) * __CHAR_BIT__ / 2 + 2) != (U) 0x101 << (sizeof (S) * __CHAR_BIT__ - 5))
+ __builtin_abort ();
+ S c = (U) 1 << (sizeof (S) * __CHAR_BIT__ - 1);
+ if ((U) f2 (c, 5) != ((U) 0x1f << (sizeof (S) * __CHAR_BIT__ - 5)))
+ __builtin_abort ();
+ if ((U) f2 (c, sizeof (S) * __CHAR_BIT__ / 2) != ((U) -1 << (sizeof (S) * __CHAR_BIT__ / 2 - 1)))
+ __builtin_abort ();
+ if ((U) f2 (c, sizeof (S) * __CHAR_BIT__ / 2 + 2) != ((U) -1 << (sizeof (S) * __CHAR_BIT__ / 2 - 3)))
+ __builtin_abort ();
+ U d = (U) 1 << (sizeof (S) * __CHAR_BIT__ - 1);
+ if (f3 (c, 5) != ((U) 0x1 << (sizeof (S) * __CHAR_BIT__ - 5)))
+ __builtin_abort ();
+ if (f3 (c, sizeof (S) * __CHAR_BIT__ / 2) != ((U) 1 << (sizeof (S) * __CHAR_BIT__ / 2 - 1)))
+ __builtin_abort ();
+ if (f3 (c, sizeof (S) * __CHAR_BIT__ / 2 + 2) != ((U) 1 << (sizeof (S) * __CHAR_BIT__ / 2 - 3)))
+ __builtin_abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-02 13:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-02 7:42 [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582) Jakub Jelinek
2018-05-02 7:50 ` Uros Bizjak
2018-05-02 8:02 ` Jakub Jelinek
2018-05-02 8:09 ` Uros Bizjak
2018-05-02 13:31 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).