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