| Message ID | 52CBEC0D.4070706@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 7 Jan 2014, Kugan wrote: > ping ? > > I have reorganised the last patch and now handling only > VIEW_CONVERT_EXPR, CONVERT_EXPR and NOP_EXPR. Once it is reviewed and > necessary changes are made, I will address the other cases as a separate > patch (when it reaches that stage). Note that VIEW_CONVERT_EXPR is wrong here. I think you are handling this wrong still. From a quick look you want to avoid the actual promotion for reg_1 = .... when reg_1 is promoted and thus the target is (subreg:XX N). The RHS has been expanded in XXmode. Dependent on the value-range of reg_1 you want to set N to a paradoxical subreg of the expanded result. You can always do that if the reg is zero-extended and else if the MSB is not set for any of the values of reg_1. I don't see how is_assigned_exp_fit_type reflects this in any way. Anyway, the patch should not introduce another if (promoted) case but only short-cut the final convert_move call of the existing one. Richard. > Thanks, > Kugan > > gcc/ > > +2014-01-07 Kugan Vivekanandarajah <kuganv@linaro.org> > + > + * dojump.c (do_compare_and_jump): Generate rtl without > + zero/sign extension if redundant. > + * cfgexpand.c (expand_gimple_stmt_1): Likewise. > + (is_assigned_exp_fit_type) : New function. > + * cfgexpand.h (is_assigned_exp_fit_type) : Declare. > + >
On 07/01/14 23:23, Richard Biener wrote: > On Tue, 7 Jan 2014, Kugan wrote: [snip] > Note that VIEW_CONVERT_EXPR is wrong here. I think you are > handling this wrong still. From a quick look you want to avoid > the actual promotion for > > reg_1 = .... > > when reg_1 is promoted and thus the target is (subreg:XX N). > The RHS has been expanded in XXmode. Dependent on the value-range > of reg_1 you want to set N to a paradoxical subreg of the expanded > result. You can always do that if the reg is zero-extended > and else if the MSB is not set for any of the values of reg_1. Thanks Richard for the explanation. I just want to double confirm I understand you correctly before I attempt to fix it. So let me try this for the following example, for a gimple stmt of the following from: unsigned short _5; short int _6; _6 = (short int)_5; ;; _6 = (short int) _5; target = (subreg/s/u:HI (reg:SI 110 [ D.4144 ]) 0) temp = (subreg:HI (reg:SI 118) 0) So, I must generate the following if it satisfies the other conditions. (set (reg:SI 110 [ D.4144 ]) (subreg:SI temp )) Is my understanding correct? > I don't see how is_assigned_exp_fit_type reflects this in any way. > What I tried doing with the patch is: (insn 13 12 0 (set (reg:SI 110 [ D.4144 ]) (zero_extend:SI (subreg:HI (reg:SI 118) 0))) c5.c:8 -1 (nil)) If the values in register (reg:SI 118) fits HI mode (without overflowing), I assume that it is not necessary to just drop the higher bits and zero_extend as done above and generate the following instead. (insn 13 12 0 (set (reg:SI 110 [ D.4144 ]) (((reg:SI 118) 0))) c5.c:8 -1 (nil)) is_assigned_exp_fit_type just checks if the range fits (in the above case, the value in eg:SI 118 fits HI mode) and the checks before emit_move_insn (SUBREG_REG (target), SUBREG_REG (temp)); checks the modes match. Is this wrong or am I missing the whole point? > Anyway, the patch should not introduce another if (promoted) > case but only short-cut the final convert_move call of the existing > one. > Thanks, Kugan
On Wed, 8 Jan 2014, Kugan wrote: > > On 07/01/14 23:23, Richard Biener wrote: > > On Tue, 7 Jan 2014, Kugan wrote: > > [snip] > > > > Note that VIEW_CONVERT_EXPR is wrong here. I think you are > > handling this wrong still. From a quick look you want to avoid > > the actual promotion for > > > > reg_1 = .... > > > > when reg_1 is promoted and thus the target is (subreg:XX N). > > The RHS has been expanded in XXmode. Dependent on the value-range > > of reg_1 you want to set N to a paradoxical subreg of the expanded > > result. You can always do that if the reg is zero-extended > > and else if the MSB is not set for any of the values of reg_1. > > Thanks Richard for the explanation. I just want to double confirm I > understand you correctly before I attempt to fix it. So let me try this > for the following example, > > for a gimple stmt of the following from: > unsigned short _5; > short int _6; > _6 = (short int)_5; > > ;; _6 = (short int) _5; > target = (subreg/s/u:HI (reg:SI 110 [ D.4144 ]) 0) > temp = (subreg:HI (reg:SI 118) 0) > > So, I must generate the following if it satisfies the other conditions. > (set (reg:SI 110 [ D.4144 ]) (subreg:SI temp )) > > Is my understanding correct? I'm no RTL expert in this particular area but yes, I think so. Not sure what paradoxical subregs are valid, so somebody else should comment here. You could even generate (set (reg:SI 110) (reg:SI 118)) iff temp is a SUBREG of a promoted var, as you require that for the destination as well. > > > I don't see how is_assigned_exp_fit_type reflects this in any way. > > > > > What I tried doing with the patch is: > > (insn 13 12 0 (set (reg:SI 110 [ D.4144 ]) > (zero_extend:SI (subreg:HI (reg:SI 118) 0))) c5.c:8 -1 > (nil)) > > If the values in register (reg:SI 118) fits HI mode (without > overflowing), I assume that it is not necessary to just drop the higher > bits and zero_extend as done above and generate the following instead. > > (insn 13 12 0 (set (reg:SI 110 [ D.4144 ]) > (((reg:SI 118) 0))) c5.c:8 -1 > (nil)) > > is_assigned_exp_fit_type just checks if the range fits (in the above > case, the value in eg:SI 118 fits HI mode) and the checks before > emit_move_insn (SUBREG_REG (target), SUBREG_REG (temp)); checks the > modes match. > > Is this wrong or am I missing the whole point? is_assigned_exp_fit_type is weird - it looks at the value-range of _5, but as you want to elide the extension from _6 to SImode you want to look at the value-range from _5. So, breaking it down and applying the promotion to GIMPLE it would look like unsigned short _5; short int _6; _6 = (short int)_5; _6_7 = (int) _6; where you want to remove the last line representing the assignment to (subreg:HI (reg:SI 110)). Whether you can do that depends on the value-range of _6, not on the value-range of _5. It's also completely independent on the operation performed on the RHS. Well. As far as I understand at least. Richard.
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 7a93975..b2e2f90 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -476,6 +476,66 @@ add_scope_conflicts_1 (basic_block bb, bitmap work, bool for_conflict) } } + +/* Check gimple assign stmt and see if zero/sign extension is + redundant. i.e. if an assignment gimple statement has RHS expression + value that can fit in LHS type, subreg and extension to fit can be + redundant. Zero/sign extensions in this case can be removed. */ + +bool +is_assigned_exp_fit_type (tree lhs) +{ + double_int type_min, type_max; + double_int min1, max1; + enum tree_code stmt_code; + tree rhs1; + gimple stmt = SSA_NAME_DEF_STMT (lhs); + + if (gimple_code (stmt) != GIMPLE_ASSIGN) + return false; + + /* We remove extension for non-pointer and integral stmts. */ + if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs)) + || POINTER_TYPE_P (TREE_TYPE (lhs))) + return false; + + stmt_code = gimple_assign_rhs_code (stmt); + rhs1 = gimple_assign_rhs1 (stmt); + type_max = tree_to_double_int (TYPE_MAX_VALUE (TREE_TYPE (lhs))); + type_min = tree_to_double_int (TYPE_MIN_VALUE (TREE_TYPE (lhs))); + + if (TREE_CODE_CLASS (stmt_code) == tcc_unary) + { + bool uns = TYPE_UNSIGNED (TREE_TYPE (rhs1)); + /* Get the value range. */ + if (TREE_CODE (rhs1) == INTEGER_CST) + { + min1 = tree_to_double_int (rhs1); + max1 = tree_to_double_int (rhs1); + } + else if (get_range_info (rhs1, &min1, &max1) != VR_RANGE) + return false; + + switch (stmt_code) + { + case VIEW_CONVERT_EXPR: + case CONVERT_EXPR: + case NOP_EXPR: + /* If rhs value range fits lhs type, zero/sign extension is + redundant. */ + if (max1.cmp (type_max, 0) != 1 + && (type_min.cmp (min1, 0)) != 1) + return true; + else + return false; + default: + return false; + } + } + + return false; +} + /* Generate stack partition conflicts between all partitions that are simultaneously live. */ @@ -3247,6 +3307,20 @@ expand_gimple_stmt_1 (gimple stmt) if (temp == target) ; + /* If the value in SUBREG of temp fits that SUBREG (does not + overflow) and is assigned to target SUBREG of the same mode + without sign conversion, we can skip the SUBREG + and extension. */ + else if (promoted + && is_assigned_exp_fit_type (lhs) + && (GET_CODE (temp) == SUBREG) + && (GET_MODE_PRECISION (GET_MODE (SUBREG_REG (temp))) + >= GET_MODE_PRECISION (GET_MODE (target))) + && (GET_MODE (SUBREG_REG (target)) + == GET_MODE (SUBREG_REG (temp)))) + { + emit_move_insn (SUBREG_REG (target), SUBREG_REG (temp)); + } else if (promoted) { int unsignedp = SUBREG_PROMOTED_UNSIGNED_P (target); diff --git a/gcc/cfgexpand.h b/gcc/cfgexpand.h index 04517a3..c7d73e8 100644 --- a/gcc/cfgexpand.h +++ b/gcc/cfgexpand.h @@ -22,5 +22,6 @@ along with GCC; see the file COPYING3. If not see extern tree gimple_assign_rhs_to_tree (gimple); extern HOST_WIDE_INT estimated_stack_frame_size (struct cgraph_node *); +extern bool is_assigned_exp_fit_type (tree lhs); #endif /* GCC_CFGEXPAND_H */ diff --git a/gcc/dojump.c b/gcc/dojump.c index 73df6d1..73a4b6b 100644 --- a/gcc/dojump.c +++ b/gcc/dojump.c @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see #include "ggc.h" #include "basic-block.h" #include "tm_p.h" +#include "cfgexpand.h" static bool prefer_and_bit_test (enum machine_mode, int); static void do_jump_by_parts_greater (tree, tree, int, rtx, rtx, int); @@ -1166,6 +1167,62 @@ do_compare_and_jump (tree treeop0, tree treeop1, enum rtx_code signed_code, type = TREE_TYPE (treeop0); mode = TYPE_MODE (type); + + /* Is zero/sign extension redundant. */ + bool op0_ext_redundant = false; + bool op1_ext_redundant = false; + + /* If promoted and the value in SUBREG of op0 fits (does not overflow), + it is a candidate for extension elimination. */ + if (GET_CODE (op0) == SUBREG && SUBREG_PROMOTED_VAR_P (op0)) + op0_ext_redundant = is_assigned_exp_fit_type (treeop0); + + /* If promoted and the value in SUBREG of op1 fits (does not overflow), + it is a candidate for extension elimination. */ + if (GET_CODE (op1) == SUBREG && SUBREG_PROMOTED_VAR_P (op1)) + op1_ext_redundant = is_assigned_exp_fit_type (treeop1); + + /* If zero/sign extension is redundant, generate RTL + for operands without zero/sign extension. */ + if ((op0_ext_redundant || TREE_CODE (treeop0) == INTEGER_CST) + && (op1_ext_redundant || TREE_CODE (treeop1) == INTEGER_CST)) + { + if ((TREE_CODE (treeop1) == INTEGER_CST) + && (!mode_signbit_p (GET_MODE (op1), op1))) + { + /* First operand is constant and signbit is not set (not + represented in RTL as a negative constant). */ + rtx new_op0 = gen_reg_rtx (GET_MODE (SUBREG_REG (op0))); + emit_move_insn (new_op0, SUBREG_REG (op0)); + op0 = new_op0; + } + else if ((TREE_CODE (treeop0) == INTEGER_CST) + && (!mode_signbit_p (GET_MODE (op0), op0))) + { + /* Other operand is constant and signbit is not set (not + represented in RTL as a negative constant). */ + rtx new_op1 = gen_reg_rtx (GET_MODE (SUBREG_REG (op1))); + + emit_move_insn (new_op1, SUBREG_REG (op1)); + op1 = new_op1; + } + else if ((TREE_CODE (treeop0) != INTEGER_CST) + && (TREE_CODE (treeop1) != INTEGER_CST) + && (GET_MODE (op0) == GET_MODE (op1)) + && (GET_MODE (SUBREG_REG (op0)) == GET_MODE (SUBREG_REG (op1)))) + { + /* If both comapre registers fits SUBREG and of the + same mode. */ + rtx new_op0 = gen_reg_rtx (GET_MODE (SUBREG_REG (op0))); + rtx new_op1 = gen_reg_rtx (GET_MODE (SUBREG_REG (op1))); + + emit_move_insn (new_op0, SUBREG_REG (op0)); + emit_move_insn (new_op1, SUBREG_REG (op1)); + op0 = new_op0; + op1 = new_op1; + } + } + if (TREE_CODE (treeop0) == INTEGER_CST && (TREE_CODE (treeop1) != INTEGER_CST || (GET_MODE_BITSIZE (mode)