| Message ID | a97a80c5-c2a9-86de-5152-cd1207036592@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Factor out division by squares and remove division around comparisons (0/2) | expand |
On 09/06/2017 03:54 AM, Jackson Woodruff wrote: > Hi all, > > This patch is split from part (1/2). It includes the patterns that have > been moved out of fold-const.c > > > It also removes an (almost entirely) redundant pattern: > > (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) > > which was only used in special cases, either with combinations > of flags like -fno-reciprocal-math -funsafe-math-optimizations > and cases where C was sNaN, or small enough to result in infinity. > > This pattern is covered by: > > (A / C1) +- (A / C2) -> (with O1 and reciprocal math) > A * (1 / C1) +- A * (1 / C2) -> > A * (1 / C1 +- 1 / C2) > > The previous pattern required funsafe-math-optimizations. > > To adjust for this case, the testcase has been updated to require O1 so > that the optimization is still performed. > > > This pattern is moved verbatim into match.pd: > > (A / C) +- (B / C) -> (A +- B) / C. > > OK for trunk? > > Jackson > > gcc/ > > 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> > > PR 71026/tree-optimization > * match.pd: Move RDIV patterns from fold-const.c > * fold-const.c (distribute_real_division): Removed. > (fold_binary_loc): Remove calls to distribute_real_divison. > > gcc/testsuite/ > > 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> > > PR 71026/tree-optimization > * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. Didn't you lose the case where the operator is MULT_EXPR rather than RDIV_EXPR? If I'm reading things correctly, arg0 and arg1 could be a RDIV_EXPR or a MULT_EXPR and we distribute both cases (as long as they are both the same code). Your match.pd pattern only handle rdiv. Now it may be the case that MULT_EXPR doesn't happen anymore in practice -- the code in fold-const is quote old and much of it was written before we regularly added tests for optimization and canonicalization. So I'm not surprised nothing in this testsuite trips over this omission. ISTM you need to either argue that the MULT_EXPR case is handled elsewhere or that it is no longer relevant. jeff
On 09/12/2017 11:43 PM, Jeff Law wrote: > On 09/06/2017 03:54 AM, Jackson Woodruff wrote: >> Hi all, >> >> This patch is split from part (1/2). It includes the patterns that have >> been moved out of fold-const.c >> >> >> It also removes an (almost entirely) redundant pattern: >> >> (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) >> >> which was only used in special cases, either with combinations >> of flags like -fno-reciprocal-math -funsafe-math-optimizations >> and cases where C was sNaN, or small enough to result in infinity. >> >> This pattern is covered by: >> >> (A / C1) +- (A / C2) -> (with O1 and reciprocal math) >> A * (1 / C1) +- A * (1 / C2) -> >> A * (1 / C1 +- 1 / C2) >> >> The previous pattern required funsafe-math-optimizations. >> >> To adjust for this case, the testcase has been updated to require O1 so >> that the optimization is still performed. >> >> >> This pattern is moved verbatim into match.pd: >> >> (A / C) +- (B / C) -> (A +- B) / C. >> >> OK for trunk? >> >> Jackson >> >> gcc/ >> >> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >> >> PR 71026/tree-optimization >> * match.pd: Move RDIV patterns from fold-const.c >> * fold-const.c (distribute_real_division): Removed. >> (fold_binary_loc): Remove calls to distribute_real_divison. >> >> gcc/testsuite/ >> >> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >> >> PR 71026/tree-optimization >> * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. > > Didn't you lose the case where the operator is MULT_EXPR rather than > RDIV_EXPR? > > If I'm reading things correctly, arg0 and arg1 could be a RDIV_EXPR or a > MULT_EXPR and we distribute both cases (as long as they are both the > same code). > > Your match.pd pattern only handle rdiv. > In the fold_plusminus_mult_expr function in fold-const.c, we still do the simplification: (A * C) +- (B * C) -> (A+-B) * C So the pattern I introduced only needs to handle the division case. > Now it may be the case that MULT_EXPR doesn't happen anymore in practice > -- the code in fold-const is quote old and much of it was written before > we regularly added tests for optimization and canonicalization. So I'm > not surprised nothing in this testsuite trips over this omission. In gcc.dg/fold-plusmult.c, we test whether 2*a + 2*a is folded into a * 4, which comes close. There are also a few tests for this in gcc.dg/tree-ssa/pr23294.c, although again, they are use constants for A and B. I verified that x * y + z * y Gets folded into y * (x + z) > > ISTM you need to either argue that the MULT_EXPR case is handled > elsewhere or that it is no longer relevant. > > jeff >
On 09/13/2017 03:22 AM, Jackson Woodruff wrote: > On 09/12/2017 11:43 PM, Jeff Law wrote: >> On 09/06/2017 03:54 AM, Jackson Woodruff wrote: >>> Hi all, >>> >>> This patch is split from part (1/2). It includes the patterns that have >>> been moved out of fold-const.c >>> >>> >>> It also removes an (almost entirely) redundant pattern: >>> >>> (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) >>> >>> which was only used in special cases, either with combinations >>> of flags like -fno-reciprocal-math -funsafe-math-optimizations >>> and cases where C was sNaN, or small enough to result in infinity. >>> >>> This pattern is covered by: >>> >>> (A / C1) +- (A / C2) -> (with O1 and reciprocal math) >>> A * (1 / C1) +- A * (1 / C2) -> >>> A * (1 / C1 +- 1 / C2) >>> >>> The previous pattern required funsafe-math-optimizations. >>> >>> To adjust for this case, the testcase has been updated to require O1 so >>> that the optimization is still performed. >>> >>> >>> This pattern is moved verbatim into match.pd: >>> >>> (A / C) +- (B / C) -> (A +- B) / C. >>> >>> OK for trunk? >>> >>> Jackson >>> >>> gcc/ >>> >>> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >>> >>> PR 71026/tree-optimization >>> * match.pd: Move RDIV patterns from fold-const.c >>> * fold-const.c (distribute_real_division): Removed. >>> (fold_binary_loc): Remove calls to distribute_real_divison. >>> >>> gcc/testsuite/ >>> >>> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >>> >>> PR 71026/tree-optimization >>> * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. >> >> Didn't you lose the case where the operator is MULT_EXPR rather than >> RDIV_EXPR? >> >> If I'm reading things correctly, arg0 and arg1 could be a RDIV_EXPR or a >> MULT_EXPR and we distribute both cases (as long as they are both the >> same code). > >> Your match.pd pattern only handle rdiv. >> > > In the fold_plusminus_mult_expr function in fold-const.c, we still do > the simplification: > > (A * C) +- (B * C) -> (A+-B) * C > > So the pattern I introduced only needs to handle the division case. OK. I was hoping that we had code elsewhere to handle the MULT case. > >> Now it may be the case that MULT_EXPR doesn't happen anymore in practice >> -- the code in fold-const is quote old and much of it was written before >> we regularly added tests for optimization and canonicalization. So I'm >> not surprised nothing in this testsuite trips over this omission. > > In gcc.dg/fold-plusmult.c, we test whether 2*a + 2*a is folded into a * > 4, which comes close. > > There are also a few tests for this in gcc.dg/tree-ssa/pr23294.c, > although again, they are use constants for A and B. Great. Thanks for verifying we've got at least some minimal coverage. Do you have commit privs? I'm going to give the patch a final looksie shortly. jeff
On 09/06/2017 03:54 AM, Jackson Woodruff wrote: > Hi all, > > This patch is split from part (1/2). It includes the patterns that have > been moved out of fold-const.c > > > It also removes an (almost entirely) redundant pattern: > > (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) > > which was only used in special cases, either with combinations > of flags like -fno-reciprocal-math -funsafe-math-optimizations > and cases where C was sNaN, or small enough to result in infinity. > > This pattern is covered by: > > (A / C1) +- (A / C2) -> (with O1 and reciprocal math) > A * (1 / C1) +- A * (1 / C2) -> > A * (1 / C1 +- 1 / C2) > > The previous pattern required funsafe-math-optimizations. > > To adjust for this case, the testcase has been updated to require O1 so > that the optimization is still performed. > > > This pattern is moved verbatim into match.pd: > > (A / C) +- (B / C) -> (A +- B) / C. > > OK for trunk? > > Jackson > > gcc/ > > 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> > > PR 71026/tree-optimization > * match.pd: Move RDIV patterns from fold-const.c > * fold-const.c (distribute_real_division): Removed. > (fold_binary_loc): Remove calls to distribute_real_divison. > > gcc/testsuite/ > > 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> > > PR 71026/tree-optimization > * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. Sorry. Just one more question here. Does the new match.pd pattern need to be conditional on flag_unsafe_math_optimizations? ISTM as written it's going to do those transformations independent of that flag. If this has already been discussed, don't hesitate to just point me at the discussion :-) Jeff
On 09/13/2017 04:45 PM, Jeff Law wrote: > On 09/06/2017 03:54 AM, Jackson Woodruff wrote: >> Hi all, >> >> This patch is split from part (1/2). It includes the patterns that have >> been moved out of fold-const.c >> >> >> It also removes an (almost entirely) redundant pattern: >> >> (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) >> >> which was only used in special cases, either with combinations >> of flags like -fno-reciprocal-math -funsafe-math-optimizations >> and cases where C was sNaN, or small enough to result in infinity. >> >> This pattern is covered by: >> >> (A / C1) +- (A / C2) -> (with O1 and reciprocal math) >> A * (1 / C1) +- A * (1 / C2) -> >> A * (1 / C1 +- 1 / C2) >> >> The previous pattern required funsafe-math-optimizations. >> >> To adjust for this case, the testcase has been updated to require O1 so >> that the optimization is still performed. >> >> >> This pattern is moved verbatim into match.pd: >> >> (A / C) +- (B / C) -> (A +- B) / C. >> >> OK for trunk? >> >> Jackson >> >> gcc/ >> >> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >> >> PR 71026/tree-optimization >> * match.pd: Move RDIV patterns from fold-const.c >> * fold-const.c (distribute_real_division): Removed. >> (fold_binary_loc): Remove calls to distribute_real_divison. >> >> gcc/testsuite/ >> >> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >> >> PR 71026/tree-optimization >> * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. > Sorry. Just one more question here. Does the new match.pd pattern need > to be conditional on flag_unsafe_math_optimizations? ISTM as written > it's going to do those transformations independent of that flag. For more context (if (flag_unsafe_math_optimizations) /* Simplify sqrt(x) * sqrt(x) -> x. */ (simplify (mult (SQRT@1 @0) @1) <- End mult (if (!HONOR_SNANS (type)) @0 ) <- End if !HONOR_SNANS ) <- End simplify (for op (plus minus) /* Simplify (A / C) +- (B / C) -> (A +- B) / C. */ (simplify (op (rdiv @0 @1) (rdiv @2 @1) ) <- End op (rdiv (op @0 @2) @1) <- End rdiv ) <- End simplify ) <- End for ... (more patterns) ... The if (flag_unsafe_math_optimizations) covers a whole sequence of transformations here which mine is a part of. I've annotated the close parens so it is clearer. I don't have commit privileges, could you commit this on my behalf if this is OK? Jackson. > > If this has already been discussed, don't hesitate to just point me at > the discussion :-) > > Jeff >
On 09/13/2017 10:25 AM, Jackson Woodruff wrote: > > > On 09/13/2017 04:45 PM, Jeff Law wrote: >> On 09/06/2017 03:54 AM, Jackson Woodruff wrote: >>> Hi all, >>> >>> This patch is split from part (1/2). It includes the patterns that have >>> been moved out of fold-const.c >>> >>> >>> It also removes an (almost entirely) redundant pattern: >>> >>> (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) >>> >>> which was only used in special cases, either with combinations >>> of flags like -fno-reciprocal-math -funsafe-math-optimizations >>> and cases where C was sNaN, or small enough to result in infinity. >>> >>> This pattern is covered by: >>> >>> (A / C1) +- (A / C2) -> (with O1 and reciprocal math) >>> A * (1 / C1) +- A * (1 / C2) -> >>> A * (1 / C1 +- 1 / C2) >>> >>> The previous pattern required funsafe-math-optimizations. >>> >>> To adjust for this case, the testcase has been updated to require O1 so >>> that the optimization is still performed. >>> >>> >>> This pattern is moved verbatim into match.pd: >>> >>> (A / C) +- (B / C) -> (A +- B) / C. >>> >>> OK for trunk? >>> >>> Jackson >>> >>> gcc/ >>> >>> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >>> >>> PR 71026/tree-optimization >>> * match.pd: Move RDIV patterns from fold-const.c >>> * fold-const.c (distribute_real_division): Removed. >>> (fold_binary_loc): Remove calls to distribute_real_divison. >>> >>> gcc/testsuite/ >>> >>> 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> >>> >>> PR 71026/tree-optimization >>> * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1. >> Sorry. Just one more question here. Does the new match.pd pattern need >> to be conditional on flag_unsafe_math_optimizations? ISTM as written >> it's going to do those transformations independent of that flag. > > For more context > > (if (flag_unsafe_math_optimizations) > /* Simplify sqrt(x) * sqrt(x) -> x. */ > (simplify > (mult (SQRT@1 @0) @1) <- End mult > (if (!HONOR_SNANS (type)) > @0 > ) <- End if !HONOR_SNANS > ) <- End simplify > > (for op (plus minus) > /* Simplify (A / C) +- (B / C) -> (A +- B) / C. */ > (simplify > (op (rdiv @0 @1) > (rdiv @2 @1) > ) <- End op > (rdiv (op @0 @2) @1) <- End rdiv > ) <- End simplify > ) <- End for > > ... (more patterns) ... > > The if (flag_unsafe_math_optimizations) covers a whole sequence of > transformations here which mine is a part of. I've annotated the close > parens so it is clearer. Ah. Missed the indention level. I'd cut-n-pasted the new bits to "verify" my suspicion as well and shoved it at the end of my match.pd. WHich of course wasn't protected. > > I don't have commit privileges, could you commit this on my behalf if > this is OK? Will do. If you're going to contribute regularly you might as well get that process started though :-) List me as your sponsor. https://gcc.gnu.org/svnwrite.html
diff --git a/gcc/fold-const.c b/gcc/fold-const.c index de60f681514aacedb993d5c83c081354fa3b342b..9de1728fb27b7749aaca1ab318b88c4c9b237317 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -3794,47 +3794,6 @@ invert_truthvalue_loc (location_t loc, tree arg) : TRUTH_NOT_EXPR, type, arg); } - -/* Knowing that ARG0 and ARG1 are both RDIV_EXPRs, simplify a binary operation - with code CODE. This optimization is unsafe. */ -static tree -distribute_real_division (location_t loc, enum tree_code code, tree type, - tree arg0, tree arg1) -{ - bool mul0 = TREE_CODE (arg0) == MULT_EXPR; - bool mul1 = TREE_CODE (arg1) == MULT_EXPR; - - /* (A / C) +- (B / C) -> (A +- B) / C. */ - if (mul0 == mul1 - && operand_equal_p (TREE_OPERAND (arg0, 1), - TREE_OPERAND (arg1, 1), 0)) - return fold_build2_loc (loc, mul0 ? MULT_EXPR : RDIV_EXPR, type, - fold_build2_loc (loc, code, type, - TREE_OPERAND (arg0, 0), - TREE_OPERAND (arg1, 0)), - TREE_OPERAND (arg0, 1)); - - /* (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2). */ - if (operand_equal_p (TREE_OPERAND (arg0, 0), - TREE_OPERAND (arg1, 0), 0) - && TREE_CODE (TREE_OPERAND (arg0, 1)) == REAL_CST - && TREE_CODE (TREE_OPERAND (arg1, 1)) == REAL_CST) - { - REAL_VALUE_TYPE r0, r1; - r0 = TREE_REAL_CST (TREE_OPERAND (arg0, 1)); - r1 = TREE_REAL_CST (TREE_OPERAND (arg1, 1)); - if (!mul0) - real_arithmetic (&r0, RDIV_EXPR, &dconst1, &r0); - if (!mul1) - real_arithmetic (&r1, RDIV_EXPR, &dconst1, &r1); - real_arithmetic (&r0, code, &r0, &r1); - return fold_build2_loc (loc, MULT_EXPR, type, - TREE_OPERAND (arg0, 0), - build_real (type, r0)); - } - - return NULL_TREE; -} /* Return a BIT_FIELD_REF of type TYPE to refer to BITSIZE bits of INNER starting at BITPOS. The field is unsigned if UNSIGNEDP is nonzero @@ -9378,12 +9337,6 @@ fold_binary_loc (location_t loc, } } - if (flag_unsafe_math_optimizations - && (TREE_CODE (arg0) == RDIV_EXPR || TREE_CODE (arg0) == MULT_EXPR) - && (TREE_CODE (arg1) == RDIV_EXPR || TREE_CODE (arg1) == MULT_EXPR) - && (tem = distribute_real_division (loc, code, type, arg0, arg1))) - return tem; - /* Convert a + (b*c + d*e) into (a + b*c) + d*e. We associate floats only if the user has specified -fassociative-math. */ @@ -9775,13 +9728,6 @@ fold_binary_loc (location_t loc, return tem; } - if (FLOAT_TYPE_P (type) - && flag_unsafe_math_optimizations - && (TREE_CODE (arg0) == RDIV_EXPR || TREE_CODE (arg0) == MULT_EXPR) - && (TREE_CODE (arg1) == RDIV_EXPR || TREE_CODE (arg1) == MULT_EXPR) - && (tem = distribute_real_division (loc, code, type, arg0, arg1))) - return tem; - /* Handle (A1 * C1) - (A2 * C2) with A1, A2 or C1, C2 being the same or one. Make sure the type is not saturating and has the signedness of the stripped operands, as fold_plusminus_mult_expr will re-associate. diff --git a/gcc/match.pd b/gcc/match.pd index 69dd8193cd0524d99fba8be8da8183230b8d621a..ab3f133f443a02e423abfbd635947ecaa8024a74 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -3517,6 +3517,13 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (!HONOR_SNANS (type)) @0)) + (for op (plus minus) + /* Simplify (A / C) +- (B / C) -> (A +- B) / C. */ + (simplify + (op (rdiv @0 @1) + (rdiv @2 @1)) + (rdiv (op @0 @2) @1))) + /* Simplify sqrt(x) * sqrt(y) -> sqrt(x*y). */ (for root (SQRT CBRT) (simplify diff --git a/gcc/testsuite/gcc.dg/fold-div-1.c b/gcc/testsuite/gcc.dg/fold-div-1.c index c1c7250f882cfed7705ba60994e47440580f4c76..73b75861166f40733d9768e9703664d51ee1a9ef 100644 --- a/gcc/testsuite/gcc.dg/fold-div-1.c +++ b/gcc/testsuite/gcc.dg/fold-div-1.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-funsafe-math-optimizations -fdump-tree-gimple" } */ +/* { dg-options "-O1 -funsafe-math-optimizations -fdump-tree-gimple" } */ float f(float x) {
Hi all, This patch is split from part (1/2). It includes the patterns that have been moved out of fold-const.c It also removes an (almost entirely) redundant pattern: (A / C1) +- (A / C2) -> A * (1 / C1 +- 1 / C2) which was only used in special cases, either with combinations of flags like -fno-reciprocal-math -funsafe-math-optimizations and cases where C was sNaN, or small enough to result in infinity. This pattern is covered by: (A / C1) +- (A / C2) -> (with O1 and reciprocal math) A * (1 / C1) +- A * (1 / C2) -> A * (1 / C1 +- 1 / C2) The previous pattern required funsafe-math-optimizations. To adjust for this case, the testcase has been updated to require O1 so that the optimization is still performed. This pattern is moved verbatim into match.pd: (A / C) +- (B / C) -> (A +- B) / C. OK for trunk? Jackson gcc/ 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> PR 71026/tree-optimization * match.pd: Move RDIV patterns from fold-const.c * fold-const.c (distribute_real_division): Removed. (fold_binary_loc): Remove calls to distribute_real_divison. gcc/testsuite/ 2017-08-30 Jackson Woodruff <jackson.woodruff@arm.com> PR 71026/tree-optimization * gcc/testsuire/gcc.dg/fold-div-1.c: Use O1.