This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [ping][PATCH][1 of 2] Add value range info to SSA_NAME for zero sign extension elimination in RTL
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Kugan <kugan dot vivekanandarajah at linaro dot org>
- Cc: Richard Biener <rguenther at suse dot de>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Eric Botcazou <ebotcazou at adacore dot com>, Ramana Radhakrishnan <ramana dot radhakrishnan at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>
- Date: Mon, 2 Sep 2013 14:45:20 +0200
- Subject: Re: [ping][PATCH][1 of 2] Add value range info to SSA_NAME for zero sign extension elimination in RTL
- Authentication-results: sourceware.org; auth=none
- References: <51ABFBDB dot 6040205 at linaro dot org> <51BE66E8 dot 3010401 at linaro dot org> <alpine dot LNX dot 2 dot 00 dot 1306171045180 dot 22313 at zhemvz dot fhfr dot qr> <51D41852 dot 9050508 at linaro dot org>
On Wed, Jul 3, 2013 at 2:25 PM, Kugan <kugan.vivekanandarajah@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