From: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>,
Wilco Dijkstra <wilco.dijkstra@arm.com>
Subject: Re: [PATCH, ARM] correctly encode the CC reg data flow
Date: Fri, 13 Jan 2017 13:50:00 -0000 [thread overview]
Message-ID: <3f5e5538-5dd3-b416-904f-b87f115336fe@arm.com> (raw)
In-Reply-To: <AM4PR0701MB2162CC520827145E96A75FE2E49E0@AM4PR0701MB2162.eurprd07.prod.outlook.com>
On 18/12/16 12:58, Bernd Edlinger wrote:
> Hi,
>
> this is related to PR77308, the follow-up patch will depend on this one.
>
> When trying the split the *arm_cmpdi_insn and *arm_cmpdi_unsigned
> before reload, a mis-compilation in libgcc function __gnu_satfractdasq
> was discovered, see [1] for more details.
>
> The reason seems to be that when the *arm_cmpdi_insn is directly
> followed by a *arm_cmpdi_unsigned instruction, both are split
> up into this:
>
> [(set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 0) (match_dup 1)))
> (parallel [(set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 3) (match_dup 4)))
> (set (match_dup 2)
> (minus:SI (match_dup 5)
> (ltu:SI (reg:CC_C CC_REGNUM) (const_int
> 0))))])]
>
> [(set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 2) (match_dup 3)))
> (cond_exec (eq:SI (reg:CC CC_REGNUM) (const_int 0))
> (set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 0) (match_dup 1))))]
>
> The problem is that the reg:CC from the *subsi3_carryin_compare
> is not mentioning that the reg:CC is also dependent on the reg:CC
> from before. Therefore the *arm_cmpsi_insn appears to be
> redundant and thus got removed, because the data values are identical.
>
> I think that applies to a number of similar pattern where data
> flow is happening through the CC reg.
>
> So this is a kind of correctness issue, and should be fixed
> independently from the optimization issue PR77308.
>
> Therefore I think the patterns need to specify the true
> value that will be in the CC reg, in order for cse to
> know what the instructions are really doing.
>
>
> Bootstrapped and reg-tested on arm-linux-gnueabihf.
> Is it OK for trunk?
>
I agree you've found a valid problem here, but I have some issues with
the patch itself.
(define_insn_and_split "subdi3_compare1"
[(set (reg:CC_NCV CC_REGNUM)
(compare:CC_NCV
(match_operand:DI 1 "register_operand" "r")
(match_operand:DI 2 "register_operand" "r")))
(set (match_operand:DI 0 "register_operand" "=&r")
(minus:DI (match_dup 1) (match_dup 2)))]
"TARGET_32BIT"
"#"
"&& reload_completed"
[(parallel [(set (reg:CC CC_REGNUM)
(compare:CC (match_dup 1) (match_dup 2)))
(set (match_dup 0) (minus:SI (match_dup 1) (match_dup 2)))])
(parallel [(set (reg:CC_C CC_REGNUM)
(compare:CC_C
(zero_extend:DI (match_dup 4))
(plus:DI (zero_extend:DI (match_dup 5))
(ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
(set (match_dup 3)
(minus:SI (minus:SI (match_dup 4) (match_dup 5))
(ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
This pattern is now no-longer self consistent in that before the split
the overall result for the condition register is in mode CC_NCV, but
afterwards it is just CC_C.
I think CC_NCV is correct mode (the N, C and V bits all correctly
reflect the result of the 64-bit comparison), but that then implies that
the cc mode of subsi3_carryin_compare is incorrect as well and should in
fact also be CC_NCV. Thinking about this pattern, I'm inclined to agree
that CC_NCV is the correct mode for this operation
I'm not sure if there are other consequences that will fall out from
fixing this (it's possible that we might need a change to select_cc_mode
as well).
R.
>
> Thanks
> Bernd.
>
>
> [1] https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00680.html
>
>
> patch-pr77308-5.diff
>
>
> 2016-12-10 Bernd Edlinger <bernd.edlinger@hotmail.de>
>
> PR target/77308
> * config/arm/arm.md (adddi3_compareV, *addsi3_compareV_upper,
> adddi3_compareC, *addsi3_compareC_upper, subdi3_compare1,
> subsi3_carryin_compare, subsi3_carryin_compare_const,
> negdi2_compare, *negsi2_carryin_compare,
> *arm_cmpdi_insn): Fix the CC reg dataflow.
>
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md (revision 243515)
> +++ gcc/config/arm/arm.md (working copy)
> @@ -669,17 +669,15 @@
> (set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2)))])
> (parallel [(set (reg:CC_V CC_REGNUM)
> (ne:CC_V
> - (plus:DI (plus:DI
> - (sign_extend:DI (match_dup 4))
> - (sign_extend:DI (match_dup 5)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> - (plus:DI (sign_extend:DI
> - (plus:SI (match_dup 4) (match_dup 5)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> - (set (match_dup 3) (plus:SI (plus:SI
> - (match_dup 4) (match_dup 5))
> - (ltu:SI (reg:CC_C CC_REGNUM)
> - (const_int 0))))])]
> + (plus:DI (plus:DI (sign_extend:DI (match_dup 4))
> + (sign_extend:DI (match_dup 5)))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> + (sign_extend:DI
> + (plus:SI (plus:SI (match_dup 4) (match_dup 5))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))))
> + (set (match_dup 3) (plus:SI (plus:SI (match_dup 4) (match_dup 5))
> + (ltu:SI (reg:CC_C CC_REGNUM)
> + (const_int 0))))])]
> "
> {
> operands[3] = gen_highpart (SImode, operands[0]);
> @@ -713,13 +711,13 @@
> [(set (reg:CC_V CC_REGNUM)
> (ne:CC_V
> (plus:DI
> - (plus:DI
> - (sign_extend:DI (match_operand:SI 1 "register_operand" "r"))
> - (sign_extend:DI (match_operand:SI 2 "register_operand" "r")))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> - (plus:DI (sign_extend:DI
> - (plus:SI (match_dup 1) (match_dup 2)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> + (plus:DI
> + (sign_extend:DI (match_operand:SI 1 "register_operand" "r"))
> + (sign_extend:DI (match_operand:SI 2 "register_operand" "r")))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> + (sign_extend:DI (plus:SI (plus:SI (match_dup 1) (match_dup 2))
> + (ltu:SI (reg:CC_C CC_REGNUM)
> + (const_int 0))))))
> (set (match_operand:SI 0 "register_operand" "=r")
> (plus:SI
> (plus:SI (match_dup 1) (match_dup 2))
> @@ -748,17 +746,15 @@
> (set (match_dup 0) (plus:SI (match_dup 1) (match_dup 2)))])
> (parallel [(set (reg:CC_C CC_REGNUM)
> (ne:CC_C
> - (plus:DI (plus:DI
> - (zero_extend:DI (match_dup 4))
> - (zero_extend:DI (match_dup 5)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> - (plus:DI (zero_extend:DI
> - (plus:SI (match_dup 4) (match_dup 5)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> - (set (match_dup 3) (plus:SI
> - (plus:SI (match_dup 4) (match_dup 5))
> - (ltu:SI (reg:CC_C CC_REGNUM)
> - (const_int 0))))])]
> + (plus:DI (plus:DI (zero_extend:DI (match_dup 4))
> + (zero_extend:DI (match_dup 5)))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> + (zero_extend:DI
> + (plus:SI (plus:SI (match_dup 4) (match_dup 5))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))))
> + (set (match_dup 3) (plus:SI (plus:SI (match_dup 4) (match_dup 5))
> + (ltu:SI (reg:CC_C CC_REGNUM)
> + (const_int 0))))])]
> "
> {
> operands[3] = gen_highpart (SImode, operands[0]);
> @@ -777,17 +773,16 @@
> [(set (reg:CC_C CC_REGNUM)
> (ne:CC_C
> (plus:DI
> - (plus:DI
> - (zero_extend:DI (match_operand:SI 1 "register_operand" "r"))
> - (zero_extend:DI (match_operand:SI 2 "register_operand" "r")))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> - (plus:DI (zero_extend:DI
> - (plus:SI (match_dup 1) (match_dup 2)))
> - (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> + (plus:DI
> + (zero_extend:DI (match_operand:SI 1 "register_operand" "r"))
> + (zero_extend:DI (match_operand:SI 2 "register_operand" "r")))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))
> + (zero_extend:DI
> + (plus:SI (plus:SI (match_dup 1) (match_dup 2))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))))
> (set (match_operand:SI 0 "register_operand" "=r")
> - (plus:SI
> - (plus:SI (match_dup 1) (match_dup 2))
> - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> + (plus:SI (plus:SI (match_dup 1) (match_dup 2))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> "TARGET_32BIT"
> "adcs%?\\t%0, %1, %2"
> [(set_attr "conds" "set")
> @@ -1086,8 +1081,8 @@
> })
>
> (define_insn_and_split "subdi3_compare1"
> - [(set (reg:CC CC_REGNUM)
> - (compare:CC
> + [(set (reg:CC_NCV CC_REGNUM)
> + (compare:CC_NCV
> (match_operand:DI 1 "register_operand" "r")
> (match_operand:DI 2 "register_operand" "r")))
> (set (match_operand:DI 0 "register_operand" "=&r")
> @@ -1098,10 +1093,14 @@
> [(parallel [(set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 1) (match_dup 2)))
> (set (match_dup 0) (minus:SI (match_dup 1) (match_dup 2)))])
> - (parallel [(set (reg:CC CC_REGNUM)
> - (compare:CC (match_dup 4) (match_dup 5)))
> - (set (match_dup 3) (minus:SI (minus:SI (match_dup 4) (match_dup 5))
> - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
> + (parallel [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (zero_extend:DI (match_dup 4))
> + (plus:DI (zero_extend:DI (match_dup 5))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> + (set (match_dup 3)
> + (minus:SI (minus:SI (match_dup 4) (match_dup 5))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
> {
> operands[3] = gen_highpart (SImode, operands[0]);
> operands[0] = gen_lowpart (SImode, operands[0]);
> @@ -1156,13 +1155,15 @@
> )
>
> (define_insn "*subsi3_carryin_compare"
> - [(set (reg:CC CC_REGNUM)
> - (compare:CC (match_operand:SI 1 "s_register_operand" "r")
> - (match_operand:SI 2 "s_register_operand" "r")))
> + [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (zero_extend:DI (match_operand:SI 1 "s_register_operand" "r"))
> + (plus:DI
> + (zero_extend:DI (match_operand:SI 2 "s_register_operand" "r"))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> (set (match_operand:SI 0 "s_register_operand" "=r")
> - (minus:SI (minus:SI (match_dup 1)
> - (match_dup 2))
> - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> + (minus:SI (minus:SI (match_dup 1) (match_dup 2))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> "TARGET_32BIT"
> "sbcs\\t%0, %1, %2"
> [(set_attr "conds" "set")
> @@ -1170,13 +1171,15 @@
> )
>
> (define_insn "*subsi3_carryin_compare_const"
> - [(set (reg:CC CC_REGNUM)
> - (compare:CC (match_operand:SI 1 "reg_or_int_operand" "r")
> - (match_operand:SI 2 "arm_not_operand" "K")))
> + [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (zero_extend:DI (match_operand:SI 1 "reg_or_int_operand" "r"))
> + (plus:DI
> + (zero_extend:DI (match_operand:SI 2 "arm_not_operand" "K"))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> (set (match_operand:SI 0 "s_register_operand" "=r")
> - (minus:SI (plus:SI (match_dup 1)
> - (match_dup 2))
> - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> + (minus:SI (plus:SI (match_dup 1) (match_dup 2))
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> "TARGET_32BIT"
> "sbcs\\t%0, %1, #%B2"
> [(set_attr "conds" "set")
> @@ -4633,8 +4636,8 @@
>
>
> (define_insn_and_split "negdi2_compare"
> - [(set (reg:CC CC_REGNUM)
> - (compare:CC
> + [(set (reg:CC_NCV CC_REGNUM)
> + (compare:CC_NCV
> (const_int 0)
> (match_operand:DI 1 "register_operand" "0,r")))
> (set (match_operand:DI 0 "register_operand" "=r,&r")
> @@ -4646,8 +4649,12 @@
> (compare:CC (const_int 0) (match_dup 1)))
> (set (match_dup 0) (minus:SI (const_int 0)
> (match_dup 1)))])
> - (parallel [(set (reg:CC CC_REGNUM)
> - (compare:CC (const_int 0) (match_dup 3)))
> + (parallel [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (const_int 0)
> + (plus:DI
> + (zero_extend:DI (match_dup 3))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> (set (match_dup 2)
> (minus:SI
> (minus:SI (const_int 0) (match_dup 3))
> @@ -4705,12 +4712,14 @@
> )
>
> (define_insn "*negsi2_carryin_compare"
> - [(set (reg:CC CC_REGNUM)
> - (compare:CC (const_int 0)
> - (match_operand:SI 1 "s_register_operand" "r")))
> + [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (const_int 0)
> + (plus:DI
> + (zero_extend:DI (match_operand:SI 1 "s_register_operand" "r"))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> (set (match_operand:SI 0 "s_register_operand" "=r")
> - (minus:SI (minus:SI (const_int 0)
> - (match_dup 1))
> + (minus:SI (minus:SI (const_int 0) (match_dup 1))
> (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> "TARGET_ARM"
> "rscs\\t%0, %1, #0"
> @@ -7359,12 +7368,15 @@
> "#" ; "cmp\\t%Q0, %Q1\;sbcs\\t%2, %R0, %R1"
> "&& reload_completed"
> [(set (reg:CC CC_REGNUM)
> - (compare:CC (match_dup 0) (match_dup 1)))
> - (parallel [(set (reg:CC CC_REGNUM)
> - (compare:CC (match_dup 3) (match_dup 4)))
> - (set (match_dup 2)
> - (minus:SI (match_dup 5)
> - (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
> + (compare:CC (match_dup 0) (match_dup 1)))
> + (parallel [(set (reg:CC_C CC_REGNUM)
> + (compare:CC_C
> + (zero_extend:DI (match_dup 3))
> + (plus:DI (zero_extend:DI (match_dup 4))
> + (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> + (set (match_dup 2)
> + (minus:SI (match_dup 5)
> + (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
> {
> operands[3] = gen_highpart (SImode, operands[0]);
> operands[0] = gen_lowpart (SImode, operands[0]);
>
next prev parent reply other threads:[~2017-01-13 13:50 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-18 13:15 Bernd Edlinger
2017-01-13 13:50 ` Richard Earnshaw (lists) [this message]
2017-01-13 16:10 ` Bernd Edlinger
2017-01-13 18:29 ` Bernd Edlinger
2017-01-18 15:43 ` Bernd Edlinger
2017-04-20 19:10 ` [PING] " Bernd Edlinger
2017-04-29 17:32 ` [PING**2] " Bernd Edlinger
2017-05-12 16:50 ` [PING**3] " Bernd Edlinger
2017-06-01 16:01 ` [PING**4] " Bernd Edlinger
[not found] ` <eb07f6a9-522b-0497-fc13-f3e4508b8277@hotmail.de>
2017-06-14 12:34 ` [PING**5] " Bernd Edlinger
[not found] ` <74eaaa44-40f0-4b12-1aec-4b9926158efe@hotmail.de>
2017-07-05 18:11 ` [PING**6] " Bernd Edlinger
2017-09-04 13:55 ` Kyrill Tkachov
2017-09-04 19:54 ` Bernd Edlinger
[not found] ` <a55cfa36-bb99-3433-f99e-c261fbe5dac1@hotmail.de>
2017-09-06 12:44 ` Bernd Edlinger
2017-09-06 12:52 ` Richard Earnshaw (lists)
2017-09-06 13:00 ` Bernd Edlinger
2017-09-06 13:17 ` Bernd Edlinger
2017-09-06 15:31 ` Kyrill Tkachov
2017-09-17 8:38 ` [PING] " Bernd Edlinger
2017-10-09 13:02 ` Richard Earnshaw (lists)
2017-10-10 19:11 ` Bernd Edlinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3f5e5538-5dd3-b416-904f-b87f115336fe@arm.com \
--to=richard.earnshaw@arm.com \
--cc=bernd.edlinger@hotmail.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@foss.arm.com \
--cc=ramana.radhakrishnan@arm.com \
--cc=wilco.dijkstra@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).