On Wed, Jul 3, 2013 at 2:25 PM, Kugan <kugan.vivekanandara...@linaro.org> wrote: > On 17/06/13 18:33, Richard Biener wrote: >> >> On Mon, 17 Jun 2013, Kugan wrote: >> +/* Extract the value range of assigned exprassion for GIMPLE_ASSIGN stmt. >> + If the extracted value range is valid, return true else return >> + false. */ >> +static bool >> +extract_exp_value_range (gimple stmt, value_range_t *vr) >> +{ >> + gcc_assert (is_gimple_assign (stmt)); >> + tree rhs1 = gimple_assign_rhs1 (stmt); >> + tree lhs = gimple_assign_lhs (stmt); >> + enum tree_code rhs_code = gimple_assign_rhs_code (stmt); >> ... >> @@ -8960,6 +9016,23 @@ simplify_stmt_using_ranges (gimple_stmt_iterator >> *gsi) >> { >> enum tree_code rhs_code = gimple_assign_rhs_code (stmt); >> tree rhs1 = gimple_assign_rhs1 (stmt); >> + tree lhs = gimple_assign_lhs (stmt); >> + >> + /* Set value range information for ssa. */ >> + if (!POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt))) >> + && (TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME) >> + && INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt))) >> + && !SSA_NAME_RANGE_INFO (lhs)) >> + { >> + value_range_t vr = VR_INITIALIZER; >> ... >> + if (extract_exp_value_range (stmt, &vr)) >> + tree_ssa_set_value_range (lhs, >> + tree_to_double_int (vr.min), >> + tree_to_double_int (vr.max), >> + vr.type == VR_RANGE); >> + } >> >> This looks overly complicated to me. In vrp_finalize you can simply do >> >> for (i = 0; i < num_vr_values; i++) >> if (vr_value[i]) >> { >> tree name = ssa_name (i); >> if (POINTER_TYPE_P (name)) >> continue; >> if (vr_value[i].type == VR_RANGE >> || vr_value[i].type == VR_ANTI_RANGE) >> tree_ssa_set_value_range (name, tree_to_double_int >> (vr_value[i].min), tree_to_double_int (vr_value[i].max), vr_value[i].type >> == VR_RANGE); >> } >> > > Thanks Richard for taking time to review it. > > I was doing something like what you are suggesting earlier but noticed some > problems and that’s the reason why I changed. > > For example, for the following testcase from the test suite, > > unsigned long l = (unsigned long)-2; > unsigned short s; > > int main () { > long t = l + 1; > s = l; > if (s != (unsigned short) -2) > abort (); > exit (0); > } > > with the following gimple stmts > > main () > { > short unsigned int s.1; > long unsigned int l.0; > > ;; basic block 2, loop depth 0 > ;; pred: ENTRY > l.0_2 = l; > s.1_3 = (short unsigned int) l.0_2; > s = s.1_3; > if (s.1_3 != 65534) > goto <bb 3>; > else > goto <bb 4>; > ;; succ: 3 > ;; 4 > > ;; basic block 3, loop depth 0 > ;; pred: 2 > abort (); > ;; succ: > > ;; basic block 4, loop depth 0 > ;; pred: 2 > exit (0); > ;; succ: > > } > > > > has the following value range. > > l.0_2: VARYING > s.1_3: [0, +INF] > > > From zero/sign extension point of view, the variable s.1_3 is expected to > have a value that will overflow (or varying) as this is what is assigned to > a smaller variable. extract_range_from_assignment initially calculates the > value range as VARYING but later changed to [0, +INF] by > extract_range_basic. What I need here is the value that will be assigned > from the rhs expression and not the value that we will have with proper > assignment.
I don't understand this. The relevant statement is s.1_3 = (short unsigned int) l.0_2; right? You have value-ranges for both s.1_3 and l.0_2 as above. And you clearly cannot optimize the truncation away (and if you could, you wond't need value-range information for that fact). > I understand that the above code of mine needs to be changed but not > convinced about the best way to do that. > > I can possibly re-factor extract_range_from_assignment to give me this > information with an additional argument. Could you kindly let me know your > preference. > > > >> >> /* SSA name annotations. */ >> >> + union vrp_info_type { >> + /* Pointer attributes used for alias analysis. */ >> + struct GTY ((tag ("TREE_SSA_PTR_INFO"))) ptr_info_def *ptr_info; >> + /* Value range attributes used for zero/sign extension elimination. >> */ >> >> /* Value range information. */ >> >> + struct GTY ((tag ("TREE_SSA_RANGE_INFO"))) range_info_def >> *range_info; >> + } GTY ((desc ("%1.def_stmt && !POINTER_TYPE_P (TREE_TYPE >> ((tree)&%1))"))) vrp; >> >> why do you need to test %1.def_stmt here? > > > > I have seen some tree_ssa_name with def_stmt NULL. Thats why I added this. > Is that something that should never happen. It should never happen - they should have a GIMPLE_NOP. +void +set_range_info (tree name, double_int min, + double_int max, bool vr_range) you have some whitespace issues here (please properly use tabs) + /* Allocate if not available. */ + if (ri == NULL) + { + ri = ggc_alloc_cleared_range_info_def (); + mark_range_info_unknown (ri); that looks superfluous to me. + SSA_NAME_RANGE_INFO (name) = ri; - /* Pointer attributes used for alias analysis. */ - struct ptr_info_def *ptr_info; + /* Value range information. */ + union vrp_info_type { + /* Pointer attributes used for alias analysis. */ + struct GTY ((tag ("0"))) ptr_info_def *ptr_info; + /* Value range attributes used for zero/sign extension elimination. */ + struct GTY ((tag ("1"))) range_info_def *range_info; + } GTY ((desc ("%1.def_stmt && !POINTER_TYPE_P (TREE_TYPE ((tree)&%1))"))) vrp; please change vrp_info_type and vrp to other names - this is not vrp specific info after all, I suggest ssa_name_info_type and info. The genric bits otherwise look ok to me, the VRP bits still look wrong (see my above question) and need explanation. Thanks, Richard. > > Thanks, > Kugan