Sorry, Should have replied to gcc-patches list.
Thanks,
bin
---------- Forwarded message ----------
From: "Bin.Cheng" <amker.ch...@gmail.com>
Date: Tue, 29 Mar 2016 03:55:04 +0800
Subject: Re: [PATCH PR69489/01]Improve tree ifcvt by storing/tracking
DR against its innermost loop bahavior if possible
To: Richard Biener <richard.guent...@gmail.com>
On 3/17/16, Richard Biener <richard.guent...@gmail.com> wrote:
> On Wed, Mar 16, 2016 at 5:17 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>> On Wed, Mar 16, 2016 at 12:20 PM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>>
>>> Hmm.
>> Hi,
>> Thanks for reviewing.
>>>
>>> + equal_p = true;
>>> + if (e1->base_address && e2->base_address)
>>> + equal_p &= operand_equal_p (e1->base_address, e2->base_address, 0);
>>> + if (e1->offset && e2->offset)
>>> + equal_p &= operand_equal_p (e1->offset, e2->offset, 0);
>>>
>>> surely better to return false early.
>>>
>>> I think we don't want this in tree-data-refs.h also because of ...
>>>
>>> @@ -615,15 +619,29 @@
>>> hash_memrefs_baserefs_and_store_DRs_read_written_info
>>> (data_reference_p a)
>>> data_reference_p *master_dr, *base_master_dr;and REALPART) before
>>> creating the DR (or adjust the equality function
>> and hashing
>>> tree ref = DR_REF (a);
>>> tree base_ref = DR_BASE_OBJECT (a);
>>> + innermost_loop_behavior *innermost = &DR_INNERMOST (a);
>>> tree ca = bb_predicate (gimple_bb (DR_STMT (a)));
>>> bool exist1, exist2;
>>>
>>> - while (TREE_CODE (ref) == COMPONENT_REF
>>> - || TREE_CODE (ref) == IMAGPART_EXPR
>>> - || TREE_CODE (ref) == REALPART_EXPR)
>>> - ref = TREE_OPERAND (ref, 0);
>>> + /* If reference in DR has innermost loop behavior and it is not
>>> + a compound memory reference, we store it to innermost_DR_map,
>>> + otherwise to ref_DR_map. */
>>> + if (TREE_CODE (ref) == COMPONENT_REF
>>> + || TREE_CODE (ref) == IMAGPART_EXPR
>>> + || TREE_CODE (ref) == REALPART_EXPR
>>> + || !(DR_BASE_ADDRESS (a) || DR_OFFSET (a)
>>> + || DR_INIT (a) || DR_STEP (a) || DR_ALIGNED_TO (a)))
>>> + {
>>> + while (TREE_CODE (ref) == COMPONENT_REF
>>> + || TREE_CODE (ref) == IMAGPART_EXPR
>>> + || TREE_CODE (ref) == REALPART_EXPR)
>>> + ref = TREE_OPERAND (ref, 0);
>>> +
>>> + master_dr = &ref_DR_map->get_or_insert (ref, &exist1);
>>> + }
>>> + else
>>> + master_dr = &innermost_DR_map->get_or_insert (innermost, &exist1);
>>>
>>> we don't want an extra hashmap but replace ref_DR_map entirely. So we'd
>>> need to
>>> strip outermost non-variant handled-components (COMPONENT_REF, IMAGPART
>>> and REALPART) before creating the DR (or adjust the equality function
>>> and hashing
>>> to disregard them which means subtracting their offset from DR_INIT.
>> I am not sure if I understand correctly. But for component reference,
>> it is the base object that we want to record/track. For example,
>>
>> for (i = 0; i < N; i++) {
>> m = *data++;
>>
>> m1 = p1->x - m;
>> m2 = p2->x + m;
>>
>> p3->y = (m1 >= m2) ? p1->y : p2->y;
>>
>> p1++;
>> p2++;
>> p3++;
>> }
>> We want to infer that reads of p1/p2 in condition statement won't trap
>> because there are unconditional reads of the structures, though the
>> unconditional reads are actual of other sub-objects. Here it is the
>> invariant part of address that we want to track.
>
> Well, the variant parts - we want to strip invariant parts as far as we can
> (offsetof (x) and offsetof (y))
>
>> Also illustrated by this example, we can't rely on data-ref analyzer
>> here. Because in gathering/scattering cases, the address could be not
>> affine at all.
>
> Sure, but that's a different issue.
>
>>>
>>> To adjust the references we collect you'd maybe could use a callback
>>> to get_references_in_stmt
>>> to adjust them.
>>>
>>> OTOH post-processing the DRs in if_convertible_loop_p_1 can be as simple
>>> as
>> Is this a part of the method you suggested above, or is it an
>> alternative one? If it's the latter, then I have below questions
>> embedded.
>
> It is an alternative to adding a hook to get_references_in_stmt and
> probably "easier".
>
>>>
>>> Index: tree-if-conv.c
>>> ===================================================================
>>> --- tree-if-conv.c (revision 234215)
>>> +++ tree-if-conv.c (working copy)
>>> @@ -1235,6 +1220,38 @@ if_convertible_loop_p_1 (struct loop *lo
>>>
>>> for (i = 0; refs->iterate (i, &dr); i++)
>>> {
>>> + tree *refp = &DR_REF (dr);
>>> + while ((TREE_CODE (*refp) == COMPONENT_REF
>>> + && TREE_OPERAND (*refp, 2) == NULL_TREE)
>>> + || TREE_CODE (*refp) == IMAGPART_EXPR
>>> + || TREE_CODE (*refp) == REALPART_EXPR)
>>> + refp = &TREE_OPERAND (*refp, 0);
>>> + if (refp != &DR_REF (dr))
>>> + {
>>> + tree saved_base = *refp;
>>> + *refp = integer_zero_node;
>>> +
>>> + if (DR_INIT (dr))
>>> + {
>>> + tree poffset;
>>> + int punsignedp, preversep, pvolatilep;
>>> + machine_mode pmode;
>>> + HOST_WIDE_INT pbitsize, pbitpos;
>>> + get_inner_reference (DR_REF (dr), &pbitsize, &pbitpos,
>>> &poffset,
>>> + &pmode, &punsignedp, &preversep,
>>> &pvolatilep,
>>> + false);
>>> + gcc_assert (poffset == NULL_TREE);
>>> +
>>> + DR_INIT (dr)
>>> + = wide_int_to_tree (ssizetype,
>>> + wi::sub (DR_INIT (dr),
>>> + pbitpos / BITS_PER_UNIT));
>>> + }
>>> +
>>> + *refp = saved_base;
>>> + DR_REF (dr) = *refp;
>>> + }
>> Looks to me the code is trying to resolve difference between two (or
>> more) component references, which is DR_INIT in the code. But DR_INIT
>> is not the only thing needs to be handled. For a structure containing
>> two sub-arrays, DR_OFFSET may be different too.
>
> Yes, but we can't say that if
>
> a->a[i]
>
> doesn't trap that then
>
> a->b[i]
>
> doesn't trap either. We can only "strip" outermost
> non-variable-offset components.
>
> But maybe I'm missing what example you are thinking of.
Hmm, this was the case I meant. What I don't understand is current
code logic does infer trap information for a.b[i] from a.a[i]. Given
below example:
struct str
{
int a[10];
int b[20];
char c;
};
void bar (struct str *);
int foo (int x, int n)
{
int i;
struct str s;
bar (&s);
for (i = 0; i < n; i++)
{
s.a[i] = s.b[i];
if (x > i)
s.b[i] = 0;
}
bar (&s);
return 0;
}
The loop is convertible because of below code in function
ifcvt_memrefs_wont_trap:
/* If a is unconditionally accessed then ... */
if (DR_RW_UNCONDITIONALLY (*master_dr))
{
/* an unconditional read won't trap. */
if (DR_IS_READ (a))
return true;
/* an unconditionaly write won't trap if the base is written
to unconditionally. */
if (base_master_dr
&& DR_BASE_W_UNCONDITIONALLY (*base_master_dr))
return PARAM_VALUE (PARAM_ALLOW_STORE_DATA_RACES);
else
{
/* or the base is know to be not readonly. */
tree base_tree = get_base_address (DR_REF (a));
if (DECL_P (base_tree)
&& decl_binds_to_current_def_p (base_tree)
&& ! TREE_READONLY (base_tree))
return PARAM_VALUE (PARAM_ALLOW_STORE_DATA_RACES);
}
}
It is the main object '&s' that is recorded in base_master_dr, so
s.b[i] is considered not trap. Even it's not, I suppose
get_base_address will give same result?
Thanks,
bin
--
Best Regards.
