[PATCH, i386]: Fix PR 82628, wrong code at -Os on x86_64-linux-gnu in the 32-bit mode

[Uros Bizjak]

On Mon, Oct 23, 2017 at 1:27 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Oct 23, 2017 at 1:07 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Mon, Oct 23, 2017 at 12:27:15PM +0200, Uros Bizjak wrote:
>>> On Mon, Oct 23, 2017 at 12:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> > On Sun, Oct 22, 2017 at 08:04:28PM +0200, Uros Bizjak wrote:
>>> >> Hello!
>>> >>
>>> >> In PR 82628 Jakub figured out that insn patterns that consume carry
>>> >> flag were not 100% correct. Due to this issue, combine is able to
>>> >> simplify various CC_REG propagations that result in invalid code.
>>> >>
>>> >> Attached patch fixes (well, mitigates) the above problem by splitting
>>> >> the double-mode compare after the reload, in the same way other
>>> >> *_doubleword patterns are handled from "the beginning of the time".
>>> >
>>> > I'm afraid this is going to haunt us sooner or later, combine isn't the
>>> > only pass that uses simplify-rtx.c infrastructure heavily and when we lie
>>> > in the RTL pattern, eventually something will be simplified wrongly.
>>> >
>>> > So, at least we'd need to use UNSPEC for the pattern, like (only lightly
>>> > tested so far) below.
>>>
>>> I agree with the above. Patterns that consume Carry flag are now
>>> marked with (plus (ltu (...)), but effectively, they behave like
>>> unspecs. So, I see no problem to change all SBB and ADC to unspec at
>>> once, similar to the change you proposed in the patch.
>>
>> So like this (addcarry/subborrow defered to a separate patch)?
>> Or do you want to use UNSPEC even for the unsigned comparison case,
>> i.e. from the patch remove the predicates.md/constraints.md part,
>> sub<mode>3_carry_ccc{,_1} and anything related to that?
>
> Looking at the attached patch, I think, this won't be necessary
> anymore. The pattern is quite important for 32bit targets, so this
> fact warrants a couple of complicated patterns.
>
>> As for addcarry/subborrow, the problem is that we expect in the pr67317*
>> tests that combine is able to notice that the CF setter sets CF to
>> unconditional 0 and matches the pattern.  With the patch I wrote
>> we end up with the combiner trying to match an insn where the CCC
>> is set from a TImode comparison:
>> (parallel [
>>         (set (reg:CC 17 flags)
>>             (compare:CC (zero_extend:TI (plus:DI (reg/v:DI 92 [ a ])
>>                         (reg/v:DI 94 [ c ])))
>>                 (zero_extend:TI (reg/v:DI 94 [ c ]))))
>>         (set (reg:DI 98)
>>             (plus:DI (reg/v:DI 92 [ a ])
>>                 (reg/v:DI 94 [ c ])))
>>     ])
>> So, either we need a define_insn_and_split pattern that would deal with
>> that (for UNSPEC it would be the same thing, have a define_insn_and_split
>> that would replace the (ltu...) with (const_int 0)), or perhaps be smarter
>> during expansion, if we see the first argument is constant 0, expand it
>> like a normal add instruction with CC setter.
>>
>> 2017-10-23  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR target/82628
>>         * config/i386/predicates.md (x86_64_dwzext_immediate_operand): New.
>>         * config/i386/constraints.md (Wf): New constraint.
>>         * config/i386/i386.md (UNSPEC_SBB): New unspec.
>>         (cmp<dwi>_doubleword): Removed.
>>         (sub<mode>3_carry_ccc, *sub<mode>3_carry_ccc_1): New patterns.
>>         (sub<mode>3_carry_ccgz): Use unspec instead of compare.
>>         * config/i386/i386.c (ix86_expand_branch) <case E_TImode>: Don't
>>         expand with cmp<dwi>_doubleword.  For LTU and GEU use
>>         sub<mode>3_carry_ccc instead of sub<mode>3_carry_ccgz and use CCCmode.
>
> OK.

The patch also fixes PR 82662. I have added the following testcase and
closed the PR.

2017-10-23  Uros Bizjak  <ubizjak@gmail.com>

    PR target/82662
    * gcc.target/i386/pr82662.c: New test.

Tested on x86_64-linux-gnu {,-m32} and committed to mailine SVN.

Uros.
-------------- next part --------------
Index: gcc.target/i386/pr82662.c
===================================================================
--- gcc.target/i386/pr82662.c	(nonexistent)
+++ gcc.target/i386/pr82662.c	(working copy)
@@ -0,0 +1,26 @@
+/* PR target/82580 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#ifdef __SIZEOF_INT128__
+typedef unsigned __int128 U;
+typedef signed __int128 S;
+#else
+typedef unsigned long long U;
+typedef signed long long S;
+#endif
+void bar (void);
+int f0 (U x, U y) { return x == y; }
+int f1 (U x, U y) { return x != y; }
+int f2 (U x, U y) { return x > y; }
+int f3 (U x, U y) { return x >= y; }
+int f4 (U x, U y) { return x < y; }
+int f5 (U x, U y) { return x <= y; }
+int f6 (S x, S y) { return x == y; }
+int f7 (S x, S y) { return x != y; }
+int f8 (S x, S y) { return x > y; }
+int f9 (S x, S y) { return x >= y; }
+int f10 (S x, S y) { return x < y; }
+int f11 (S x, S y) { return x <= y; }
+
+/* { dg-final { scan-assembler-times {\mset} 12 } } */