On Mon, Oct 23, 2017 at 12:09 PM, Jakub Jelinek <ja...@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.
> I'm not sure the double-word pattern is a win though, it causes PR82662
> you've filed (the problem is that during ifcvt because of the double-word
> comparison the condition is canonicalized as (lt (reg:TI) (reg:TI)) and
> there is no instruction in the MD that would take such arguments, there
> are only instructions that compare flags registers.
It is not a win, my patch was more of a band-aid to mitigate the
failure. It works, but it produces extra moves (as you mentione
below), due to RA not knowing that CMP doesn't clobber the register.
But, let's change the pattern back to expand-time splitting after the
above patch that changes SBB and ADC to unspecs is committed.
> If you look at say normal DImode comparisons, it is the same thing,
> ifcvt also can't do anything with those, the reason they work is that we
> have a cstoredi4 optab (for 64-bit), but don't have a cstoreti4 optab.
> So, we'd need that (and only handle the GE/GEU/LT/LTU + the others that can
> be handled by swapping the operands).
> I think the double-word pattern has other issues, it will result in RA not
> knowing in detail what is going on and thus can at least reserve one extra
> register that otherwise would not be needed. The reason we have the
> doubleword patterns elsewhere is that splitting double-word early makes it
> harder/impossible for STV to use SSE registers; in this case we don't have
> something reasonable to expand to anyway, we always split.
>
> The alternative I have is the patch attached in the PR, if the unrelated
> addcarry/subborrow changes are removed, then it doesn't regress anything,
> the pr50038.c FAIL is from some other earlier change even on vanilla
> branch and pr67317-* FAILs were caused by the addcarry/subborrow changes,
> will look at those in detail.
I do have patch that allows double-mode for cstore, but it is not an
elegant solution. Splitting to SBB at expand time would be
considerably better.
Thanks,
Uros.
> 2017-10-23 Jakub Jelinek <ja...@redhat.com>
>
> PR target/82628
> * config/i386/i386.md (UNSPEC_SBB): New unspec.
> (cmp<dwi>_doubleword): Use unspec instead of compare.
> (sub<mode>3_carry_ccgz): Use unspec instead of compare.
>
> --- gcc/config/i386/i386.md.jj 2017-10-23 10:13:05.462218947 +0200
> +++ gcc/config/i386/i386.md 2017-10-23 11:07:55.470376791 +0200
> @@ -112,6 +112,7 @@ (define_c_enum "unspec" [
> UNSPEC_STOS
> UNSPEC_PEEPSIB
> UNSPEC_INSN_FALSE_DEP
> + UNSPEC_SBB
>
> ;; For SSE/MMX support:
> UNSPEC_FIX_NOTRUNC
> @@ -1285,11 +1286,10 @@ (define_insn_and_split "cmp<dwi>_doublew
> [(set (reg:CC FLAGS_REG)
> (compare:CC (match_dup 1) (match_dup 2)))
> (parallel [(set (reg:CCGZ FLAGS_REG)
> - (compare: CCGZ
> - (match_dup 4)
> - (plus:DWIH
> - (ltu:DWIH (reg:CC FLAGS_REG) (const_int 0))
> - (match_dup 5))))
> + (unspec:CCGZ [(match_dup 4)
> + (match_dup 5)
> + (ltu:DWIH (reg:CC FLAGS_REG) (const_int 0))]
> + UNSPEC_SBB))
> (clobber (match_dup 3))])]
> "split_double_mode (<DWI>mode, &operands[0], 3, &operands[0],
> &operands[3]);")
>
> @@ -6911,13 +6911,18 @@ (define_insn "*subsi3_carry_zext"
> (set_attr "pent_pair" "pu")
> (set_attr "mode" "SI")])
>
> +;; The sign flag is set from the
> +;; (compare (match_dup 1) (plus:DWIH (ltu:DWIH ...) (match_dup 2)))
> +;; result, the overflow flag likewise, but the overflow flag is also
> +;; set if the (plus:DWIH (ltu:DWIH ...) (match_dup 2)) overflows.
> +;; The borrow flag can be modelled, but differently from SF and OF
> +;; and is quite difficult to handle.
> (define_insn "*sub<mode>3_carry_ccgz"
> [(set (reg:CCGZ FLAGS_REG)
> - (compare:CCGZ
> - (match_operand:DWIH 1 "register_operand" "0")
> - (plus:DWIH
> - (ltu:DWIH (reg:CC FLAGS_REG) (const_int 0))
> - (match_operand:DWIH 2 "x86_64_general_operand" "rme"))))
> + (unspec:CCGZ [(match_operand:DWIH 1 "register_operand" "0")
> + (match_operand:DWIH 2 "x86_64_general_operand" "rme")
> + (ltu:DWIH (reg:CC FLAGS_REG) (const_int 0))]
> + UNSPEC_SBB))
> (clobber (match_scratch:DWIH 0 "=r"))]
> ""
> "sbb{<imodesuffix>}\t{%2, %0|%0, %2}"
>
>
> Jakub
