Message ID | 20161031195610.GA3558@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2016-10/msg02525.html On Mon, Oct 31, 2016 at 08:56:10PM +0100, Dominik Vogt wrote: > The attached patch does a little change in > combine.c:combine_simplify_rtx() to prevent a "simplification" > where the rtl code gets more complex in reality. The complete > description of the change can be found in the commit comment in > the attached patch. > > The patch reduces the number of patterns in the s390 backend and > slightly reduces the size of the compiled SPEC2006 code. (Code > size or runtime only tested on s390x with -m64.) It is > theoretically possible that this patch leads to somewhat worse > code on some target if that only has a pattern for the formerly replaced > rtl expression but not for the original one. > > The patch has passed the testsuite on s390, s390x biarch, x86_64 > and Power biarch. > > -- > > (I'm not sure whether the const_int expression can appear in both > operands or only as the second. If the latter is the case, the > conditions can be simplified a bit.) > > What do you think about this patch? Ciao Dominik ^_^ ^_^
On 10/31/2016 08:56 PM, Dominik Vogt wrote: > combine_simplify_rtx() tries to replace rtx expressions with just two > possible values with an experession that uses if_then_else: > > (if_then_else (condition) (value1) (value2)) > > If the original expression is e.g. > > (and (reg) (const_int 2)) I'm not convinced that if_then_else_cond is the right place to do this. That function is designed to answer the question of whether an rtx has exactly one of two values and under which condition; I feel it should continue to work this way. Maybe simplify_ternary_expression needs to be taught to deal with this case? Bernd
On Mon, Nov 07, 2016 at 09:29:26PM +0100, Bernd Schmidt wrote: > On 10/31/2016 08:56 PM, Dominik Vogt wrote: > > >combine_simplify_rtx() tries to replace rtx expressions with just two > >possible values with an experession that uses if_then_else: > > > > (if_then_else (condition) (value1) (value2)) > > > >If the original expression is e.g. > > > > (and (reg) (const_int 2)) > > I'm not convinced that if_then_else_cond is the right place to do > this. That function is designed to answer the question of whether an > rtx has exactly one of two values and under which condition; I feel > it should continue to work this way. > > Maybe simplify_ternary_expression needs to be taught to deal with this case? But simplify_ternary_expression isn't called with the following test program (only tried it on s390x): void bar(int, int); int foo(int a, int *b) { if (a) bar(0, *b & 2); return *b; } combine_simplify_rtx() is called with (sign_extend:DI (and:SI (reg:SI 61) (const_int 2))) In the switch it calls simplify_unary_operation(), which return NULL. The next thing it does is call if_then_else_cond(), and that calls itself with the sign_extend peeled off: (and:SI (reg:SI 61) (const_int 2)) takes the "BINARY_P (x)" path and returns false. The problem exists only if the (and ...) is wrapped in ..._extend, i.e. the ondition dealing with (and ...) directly can be removed from the patch. So, all recursive calls to if_then_els_cond() return false, and finally the condition in else if (HWI_COMPUTABLE_MODE_P (mode) && pow2p_hwi (nz = nonzero_bits (x, mode)) is true. Thus, if if_then_else_cond should remain unchanged, the only place to fix this would be after the call to if_then_else_cond() in combine_simplify_rtx(). Actually, there already is some special case handling to override the return code of if_then_else_cond(): cond = if_then_else_cond (x, &true_rtx, &false_rtx); if (cond != 0 /* If everything is a comparison, what we have is highly unlikely to be simpler, so don't use it. */ ---> && ! (COMPARISON_P (x) && (COMPARISON_P (true_rtx) || COMPARISON_P (false_rtx)))) { rtx cop1 = const0_rtx; enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); ---> if (cond_code == NE && COMPARISON_P (cond)) return x; ... Should be easy to duplicate the test in the if-body, if that is what you prefer: ... if (HWI_COMPUTABLE_MODE_P (GET_MODE (x)) && pow2p_hwi (nz = nonzero_bits (x, GET_MODE (x))) && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) && GET_CODE (XEXP (x, 0)) == AND && CONST_INT_P (XEXP (XEXP (x, 0), 0)) && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) return x; (untested) Ciao Dominik ^_^ ^_^
diff --git a/gcc/combine.c b/gcc/combine.c index b22a274..244669d 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -9103,9 +9103,25 @@ if_then_else_cond (rtx x, rtx *ptrue, rtx *pfalse) return x; } - /* Likewise for 0 or a single bit. */ + /* Likewise for 0 or a single bit. + If the operation is an AND (possibly wrapped in a SIGN_EXTEND or + ZERO_EXTEND) with either operand being just a constant single bit value, + do nothing since IF_THEN_ELSE is likely to increase the expression's + complexity. */ else if (HWI_COMPUTABLE_MODE_P (mode) - && pow2p_hwi (nz = nonzero_bits (x, mode))) + && pow2p_hwi (nz = nonzero_bits (x, mode)) + && ! (code == AND + && ((CONST_INT_P (XEXP (x, 0)) + && UINTVAL (XEXP (x, 0)) == nz) + || (CONST_INT_P (XEXP (x, 1)) + && UINTVAL (XEXP (x, 1)) == nz))) + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) + && GET_CODE (XEXP (x, 0)) == AND + && ((CONST_INT_P (XEXP (XEXP (x, 0), 0)) + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz) + || (CONST_INT_P (XEXP (XEXP (x, 0), 1)) + && UINTVAL (XEXP (XEXP (x, 0), 1)) == nz))) + ) { *ptrue = gen_int_mode (nz, mode), *pfalse = const0_rtx; return x;