* [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types @ 2015-07-20 14:32 Kyrill Tkachov 2015-07-21 9:09 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Kyrill Tkachov @ 2015-07-20 14:32 UTC (permalink / raw) To: GCC Patches; +Cc: Richard Biener [-- Attachment #1: Type: text/plain, Size: 537 bytes --] Hi all, This patch fixes the PR in question which is a miscompilation of gcc.dg/fixed-point/unary.c on arm. It just restricts the A - B -> A + (-B) transformation when the type is fixed-point. This fixes the testcase for me. Is this the right approach? Bootstrap and test on arm and x86 running. Ok if testing is clean? Thanks, Kyrill 2015-07-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR middle-end/66915 * match.pd (A - B -> A + (-B)): Don't allow folding when type if a fixed-point type. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: matchpd-fixed-point.patch --] [-- Type: text/x-patch; name=matchpd-fixed-point.patch, Size: 692 bytes --] commit c6669b5cde3d7b504aec388282e7af955af58681 Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Mon Jul 20 15:02:17 2015 +0100 [match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types diff --git a/gcc/match.pd b/gcc/match.pd index 4427000..3d7b32e 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -522,8 +522,8 @@ along with GCC; see the file COPYING3. If not see /* A - B -> A + (-B) if B is easily negatable. */ (simplify (minus @0 negate_expr_p@1) - (plus @0 (negate @1))) - + (if (!FIXED_POINT_TYPE_P (type)) + (plus @0 (negate @1)))) /* Try to fold (type) X op CST -> (type) (X op ((type-x) CST)) when profitable. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-20 14:32 [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types Kyrill Tkachov @ 2015-07-21 9:09 ` Richard Biener 2015-07-21 9:26 ` Kyrill Tkachov 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-07-21 9:09 UTC (permalink / raw) To: Kyrill Tkachov; +Cc: GCC Patches On Mon, 20 Jul 2015, Kyrill Tkachov wrote: > Hi all, > > This patch fixes the PR in question which is a miscompilation of > gcc.dg/fixed-point/unary.c on arm. > It just restricts the A - B -> A + (-B) transformation when the type is > fixed-point. > > This fixes the testcase for me. > Is this the right approach? > > Bootstrap and test on arm and x86 running. > > Ok if testing is clean? Ok, but I think the fold-const.c code has the same issue, no: /* A - B -> A + (-B) if B is easily negatable. */ if (negate_expr_p (arg1) && !TYPE_OVERFLOW_SANITIZED (type) && ((FLOAT_TYPE_P (type) /* Avoid this transformation if B is a positive REAL_CST. */ && (TREE_CODE (arg1) != REAL_CST || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) || INTEGRAL_TYPE_P (type))) return fold_build2_loc (loc, PLUS_EXPR, type, fold_convert_loc (loc, type, arg0), fold_convert_loc (loc, type, negate_expr (arg1))); ah, no. The above only applies to float-type and integral-types. Thus yes, your patch is ok. Can you double-check the other pattern, /* -(A + B) -> (-B) - A. */ (simplify (negate (plus:c @0 negate_expr_p@1)) (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) && !HONOR_SIGNED_ZEROS (element_mode (type))) (minus (negate @1) @0))) ? Thanks, Richard. > Thanks, > Kyrill > > > 2015-07-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR middle-end/66915 > * match.pd (A - B -> A + (-B)): Don't allow folding > when type if a fixed-point type. > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-21 9:09 ` Richard Biener @ 2015-07-21 9:26 ` Kyrill Tkachov 2015-07-21 10:23 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Kyrill Tkachov @ 2015-07-21 9:26 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches On 21/07/15 08:24, Richard Biener wrote: > On Mon, 20 Jul 2015, Kyrill Tkachov wrote: > >> Hi all, >> >> This patch fixes the PR in question which is a miscompilation of >> gcc.dg/fixed-point/unary.c on arm. >> It just restricts the A - B -> A + (-B) transformation when the type is >> fixed-point. >> >> This fixes the testcase for me. >> Is this the right approach? >> >> Bootstrap and test on arm and x86 running. >> >> Ok if testing is clean? > Ok, but I think the fold-const.c code has the same issue, no: > > /* A - B -> A + (-B) if B is easily negatable. */ > if (negate_expr_p (arg1) > && !TYPE_OVERFLOW_SANITIZED (type) > && ((FLOAT_TYPE_P (type) > /* Avoid this transformation if B is a positive REAL_CST. > */ > && (TREE_CODE (arg1) != REAL_CST > || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) > || INTEGRAL_TYPE_P (type))) > return fold_build2_loc (loc, PLUS_EXPR, type, > fold_convert_loc (loc, type, arg0), > fold_convert_loc (loc, type, > negate_expr (arg1))); > > ah, no. The above only applies to float-type and integral-types. > > Thus yes, your patch is ok. Can you double-check the other pattern, > > /* -(A + B) -> (-B) - A. */ > (simplify > (negate (plus:c @0 negate_expr_p@1)) > (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) > && !HONOR_SIGNED_ZEROS (element_mode (type))) > (minus (negate @1) @0))) > > ? Thanks, committed with r226028. I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition. That would more closely mirror the original logic, right? That passes x86_64 bootstrap and aarch64 testing looks ok. > > Thanks, > Richard. > >> Thanks, >> Kyrill >> >> >> 2015-07-20 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR middle-end/66915 >> * match.pd (A - B -> A + (-B)): Don't allow folding >> when type if a fixed-point type. >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-21 9:26 ` Kyrill Tkachov @ 2015-07-21 10:23 ` Richard Biener 2015-07-23 9:21 ` Kyrill Tkachov 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-07-21 10:23 UTC (permalink / raw) To: Kyrill Tkachov; +Cc: GCC Patches On Tue, 21 Jul 2015, Kyrill Tkachov wrote: > > On 21/07/15 08:24, Richard Biener wrote: > > On Mon, 20 Jul 2015, Kyrill Tkachov wrote: > > > > > Hi all, > > > > > > This patch fixes the PR in question which is a miscompilation of > > > gcc.dg/fixed-point/unary.c on arm. > > > It just restricts the A - B -> A + (-B) transformation when the type is > > > fixed-point. > > > > > > This fixes the testcase for me. > > > Is this the right approach? > > > > > > Bootstrap and test on arm and x86 running. > > > > > > Ok if testing is clean? > > Ok, but I think the fold-const.c code has the same issue, no: > > > > /* A - B -> A + (-B) if B is easily negatable. */ > > if (negate_expr_p (arg1) > > && !TYPE_OVERFLOW_SANITIZED (type) > > && ((FLOAT_TYPE_P (type) > > /* Avoid this transformation if B is a positive REAL_CST. > > */ > > && (TREE_CODE (arg1) != REAL_CST > > || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) > > || INTEGRAL_TYPE_P (type))) > > return fold_build2_loc (loc, PLUS_EXPR, type, > > fold_convert_loc (loc, type, arg0), > > fold_convert_loc (loc, type, > > negate_expr (arg1))); > > > > ah, no. The above only applies to float-type and integral-types. > > > > Thus yes, your patch is ok. Can you double-check the other pattern, > > > > /* -(A + B) -> (-B) - A. */ > > (simplify > > (negate (plus:c @0 negate_expr_p@1)) > > (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) > > && !HONOR_SIGNED_ZEROS (element_mode (type))) > > (minus (negate @1) @0))) > > > > ? > > Thanks, committed with r226028. > I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition. > That would more closely mirror the original logic, right? > That passes x86_64 bootstrap and aarch64 testing looks ok. Yeah, that works for me, too. Thanks, Richard. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-21 10:23 ` Richard Biener @ 2015-07-23 9:21 ` Kyrill Tkachov 2015-07-23 11:19 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Kyrill Tkachov @ 2015-07-23 9:21 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches [-- Attachment #1: Type: text/plain, Size: 2378 bytes --] On 21/07/15 11:11, Richard Biener wrote: > On Tue, 21 Jul 2015, Kyrill Tkachov wrote: > >> On 21/07/15 08:24, Richard Biener wrote: >>> On Mon, 20 Jul 2015, Kyrill Tkachov wrote: >>> >>>> Hi all, >>>> >>>> This patch fixes the PR in question which is a miscompilation of >>>> gcc.dg/fixed-point/unary.c on arm. >>>> It just restricts the A - B -> A + (-B) transformation when the type is >>>> fixed-point. >>>> >>>> This fixes the testcase for me. >>>> Is this the right approach? >>>> >>>> Bootstrap and test on arm and x86 running. >>>> >>>> Ok if testing is clean? >>> Ok, but I think the fold-const.c code has the same issue, no: >>> >>> /* A - B -> A + (-B) if B is easily negatable. */ >>> if (negate_expr_p (arg1) >>> && !TYPE_OVERFLOW_SANITIZED (type) >>> && ((FLOAT_TYPE_P (type) >>> /* Avoid this transformation if B is a positive REAL_CST. >>> */ >>> && (TREE_CODE (arg1) != REAL_CST >>> || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) >>> || INTEGRAL_TYPE_P (type))) >>> return fold_build2_loc (loc, PLUS_EXPR, type, >>> fold_convert_loc (loc, type, arg0), >>> fold_convert_loc (loc, type, >>> negate_expr (arg1))); >>> >>> ah, no. The above only applies to float-type and integral-types. >>> >>> Thus yes, your patch is ok. Can you double-check the other pattern, >>> >>> /* -(A + B) -> (-B) - A. */ >>> (simplify >>> (negate (plus:c @0 negate_expr_p@1)) >>> (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) >>> && !HONOR_SIGNED_ZEROS (element_mode (type))) >>> (minus (negate @1) @0))) >>> >>> ? >> Thanks, committed with r226028. >> I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition. >> That would more closely mirror the original logic, right? >> That passes x86_64 bootstrap and aarch64 testing looks ok. > Yeah, that works for me, too. How about this patch then? Bootstrapped and tested on x86_64 and aarch64. Thanks, Kyrill 2015-07-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * match.pd (-(A + B) -> (-B) - A): Restrict to floating point and integral types. > > Thanks, > Richard. > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: matchpd-integral-float.patch --] [-- Type: text/x-patch; name=matchpd-integral-float.patch, Size: 750 bytes --] commit d514c81a7965fd24b9d8c294b12179b2369c8aa4 Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Tue Jul 21 10:18:31 2015 +0100 [match.pd] Restrict -(A + B) -> (-B) - A to integral or float types diff --git a/gcc/match.pd b/gcc/match.pd index 3d7b32e..29367f2 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -515,7 +515,8 @@ along with GCC; see the file COPYING3. If not see /* -(A + B) -> (-B) - A. */ (simplify (negate (plus:c @0 negate_expr_p@1)) - (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) + (if ((FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) + && !HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) && !HONOR_SIGNED_ZEROS (element_mode (type))) (minus (negate @1) @0))) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-23 9:21 ` Kyrill Tkachov @ 2015-07-23 11:19 ` Richard Biener 2015-07-23 12:07 ` Kyrill Tkachov 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-07-23 11:19 UTC (permalink / raw) To: Kyrill Tkachov; +Cc: GCC Patches On Thu, 23 Jul 2015, Kyrill Tkachov wrote: > > On 21/07/15 11:11, Richard Biener wrote: > > On Tue, 21 Jul 2015, Kyrill Tkachov wrote: > > > > > On 21/07/15 08:24, Richard Biener wrote: > > > > On Mon, 20 Jul 2015, Kyrill Tkachov wrote: > > > > > > > > > Hi all, > > > > > > > > > > This patch fixes the PR in question which is a miscompilation of > > > > > gcc.dg/fixed-point/unary.c on arm. > > > > > It just restricts the A - B -> A + (-B) transformation when the type > > > > > is > > > > > fixed-point. > > > > > > > > > > This fixes the testcase for me. > > > > > Is this the right approach? > > > > > > > > > > Bootstrap and test on arm and x86 running. > > > > > > > > > > Ok if testing is clean? > > > > Ok, but I think the fold-const.c code has the same issue, no: > > > > > > > > /* A - B -> A + (-B) if B is easily negatable. */ > > > > if (negate_expr_p (arg1) > > > > && !TYPE_OVERFLOW_SANITIZED (type) > > > > && ((FLOAT_TYPE_P (type) > > > > /* Avoid this transformation if B is a positive > > > > REAL_CST. > > > > */ > > > > && (TREE_CODE (arg1) != REAL_CST > > > > || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) > > > > || INTEGRAL_TYPE_P (type))) > > > > return fold_build2_loc (loc, PLUS_EXPR, type, > > > > fold_convert_loc (loc, type, arg0), > > > > fold_convert_loc (loc, type, > > > > negate_expr (arg1))); > > > > > > > > ah, no. The above only applies to float-type and integral-types. > > > > > > > > Thus yes, your patch is ok. Can you double-check the other pattern, > > > > > > > > /* -(A + B) -> (-B) - A. */ > > > > (simplify > > > > (negate (plus:c @0 negate_expr_p@1)) > > > > (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) > > > > && !HONOR_SIGNED_ZEROS (element_mode (type))) > > > > (minus (negate @1) @0))) > > > > > > > > ? > > > Thanks, committed with r226028. > > > I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the > > > condition. > > > That would more closely mirror the original logic, right? > > > That passes x86_64 bootstrap and aarch64 testing looks ok. > > Yeah, that works for me, too. > > How about this patch then? > Bootstrapped and tested on x86_64 and aarch64. Hmm. The code already pretty much matches the one in fold-const.c. So what's the actual issue with fixed-point types and -(A + B) -> -B - A iff negate_expr_p says that B can be safely negated? That is, can you add a testcase that fails without the patch? Thanks Richard. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types 2015-07-23 11:19 ` Richard Biener @ 2015-07-23 12:07 ` Kyrill Tkachov 0 siblings, 0 replies; 7+ messages in thread From: Kyrill Tkachov @ 2015-07-23 12:07 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches On 23/07/15 12:16, Richard Biener wrote: > On Thu, 23 Jul 2015, Kyrill Tkachov wrote: > >> On 21/07/15 11:11, Richard Biener wrote: >>> On Tue, 21 Jul 2015, Kyrill Tkachov wrote: >>> >>>> On 21/07/15 08:24, Richard Biener wrote: >>>>> On Mon, 20 Jul 2015, Kyrill Tkachov wrote: >>>>> >>>>>> Hi all, >>>>>> >>>>>> This patch fixes the PR in question which is a miscompilation of >>>>>> gcc.dg/fixed-point/unary.c on arm. >>>>>> It just restricts the A - B -> A + (-B) transformation when the type >>>>>> is >>>>>> fixed-point. >>>>>> >>>>>> This fixes the testcase for me. >>>>>> Is this the right approach? >>>>>> >>>>>> Bootstrap and test on arm and x86 running. >>>>>> >>>>>> Ok if testing is clean? >>>>> Ok, but I think the fold-const.c code has the same issue, no: >>>>> >>>>> /* A - B -> A + (-B) if B is easily negatable. */ >>>>> if (negate_expr_p (arg1) >>>>> && !TYPE_OVERFLOW_SANITIZED (type) >>>>> && ((FLOAT_TYPE_P (type) >>>>> /* Avoid this transformation if B is a positive >>>>> REAL_CST. >>>>> */ >>>>> && (TREE_CODE (arg1) != REAL_CST >>>>> || REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1)))) >>>>> || INTEGRAL_TYPE_P (type))) >>>>> return fold_build2_loc (loc, PLUS_EXPR, type, >>>>> fold_convert_loc (loc, type, arg0), >>>>> fold_convert_loc (loc, type, >>>>> negate_expr (arg1))); >>>>> >>>>> ah, no. The above only applies to float-type and integral-types. >>>>> >>>>> Thus yes, your patch is ok. Can you double-check the other pattern, >>>>> >>>>> /* -(A + B) -> (-B) - A. */ >>>>> (simplify >>>>> (negate (plus:c @0 negate_expr_p@1)) >>>>> (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)) >>>>> && !HONOR_SIGNED_ZEROS (element_mode (type))) >>>>> (minus (negate @1) @0))) >>>>> >>>>> ? >>>> Thanks, committed with r226028. >>>> I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the >>>> condition. >>>> That would more closely mirror the original logic, right? >>>> That passes x86_64 bootstrap and aarch64 testing looks ok. >>> Yeah, that works for me, too. >> How about this patch then? >> Bootstrapped and tested on x86_64 and aarch64. > Hmm. The code already pretty much matches the one in fold-const.c. > > So what's the actual issue with fixed-point types and > -(A + B) -> -B - A iff negate_expr_p says that B can be > safely negated? > > That is, can you add a testcase that fails without the patch? I don't have such a testcase. If negate_expr_p does what we want here, then I suppose it's redundant and I withdraw the patch. I'm not very familiar with the fold-const.c code... Kyrill > > Thanks > Richard. > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-23 11:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-20 14:32 [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types Kyrill Tkachov 2015-07-21 9:09 ` Richard Biener 2015-07-21 9:26 ` Kyrill Tkachov 2015-07-21 10:23 ` Richard Biener 2015-07-23 9:21 ` Kyrill Tkachov 2015-07-23 11:19 ` Richard Biener 2015-07-23 12:07 ` Kyrill Tkachov
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).