Re: Tweak BB analysis for dr_analyze_innermost

[Richard Sandiford <richard dot sandiford at linaro dot org>]

"Bin.Cheng" <amker.cheng@gmail.com> writes:
> On Wed, Jun 28, 2017 at 3:04 PM, Richard Sandiford
> <richard.sandiford@linaro.org> wrote:
>> "Bin.Cheng" <amker.cheng@gmail.com> writes:
>>> On Wed, Jun 28, 2017 at 2:36 PM, Richard Sandiford
>>> <richard.sandiford@linaro.org> wrote:
>>>> Index: gcc/tree-data-ref.c
>>>> ===================================================================
>>>> --- gcc/tree-data-ref.c 2017-06-28 14:33:41.294720044 +0100
>>>> +++ gcc/tree-data-ref.c 2017-06-28 14:35:30.475155670 +0100
>>>> @@ -749,15 +749,29 @@ canonicalize_base_object_address (tree a
>>>>    return build_fold_addr_expr (TREE_OPERAND (addr, 0));
>>>>  }
>>>>
>>>> -/* Analyzes the behavior of the memory reference DR in the innermost loop or
>>>> -   basic block that contains it.  Returns true if analysis succeed or false
>>>> -   otherwise.  */
>>>> +/* Analyze the behavior of memory reference DR.  There are two modes:
>>>> +
>>>> +   - BB analysis.  In this case we simply split the address into base,
>>>> +     init and offset components, without reference to any containing loop.
>>>> +     The resulting base and offset are general expressions and they can
>>>> +     vary arbitrarily from one iteration of the containing loop to the next.
>>>> +     The step is always zero.
>>>> +
>>>> +   - loop analysis.  In this case we analyze the reference both wrt LOOP
>>>> +     and on the basis that the reference occurs (is "used") in LOOP;
>>>> +     see the comment above analyze_scalar_evolution_in_loop for more
>>>> +     information about this distinction.  The base, init, offset and
>>>> +     step fields are all invariant in LOOP.
>>>> +
>>>> +   Perform BB analysis if LOOP is null, or if LOOP is the function's
>>>> +   dummy outermost loop.  In other cases perform loop analysis.
>>>> +
>>>> +   Return true if the analysis succeeded and store the results in DR if so.
>>>> +   BB analysis can only fail for bitfield or reversed-storage accesses.  */
>>>>
>>>>  bool
>>>> -dr_analyze_innermost (struct data_reference *dr, struct loop *nest)
>>>> +dr_analyze_innermost (struct data_reference *dr, struct loop *loop)
>>>>  {
>>>> -  gimple *stmt = DR_STMT (dr);
>>>> -  struct loop *loop = loop_containing_stmt (stmt);
>>>>    tree ref = DR_REF (dr);
>>>>    HOST_WIDE_INT pbitsize, pbitpos;
>>>>    tree base, poffset;
>>>> @@ -806,22 +820,11 @@ dr_analyze_innermost (struct data_refere
>>>>
>>>>    if (in_loop)
>>> A nit.  No need to use bool variable now?  We can check loop != NULL instead.
>>
>> As the comment says, we also want to do BB analysis if the loop is the
>> outermost dummy loop, so in_loop remains set to:
>>
>>   bool in_loop = (loop && loop->num);
>>
>> I think it's worth keeping that abstraction.
> Sorry if I misunderstand this.   "loop != NULL" is different to
> in_loop only when loop is the outermost dummy loop.  Which means nest
> != NULL based on change:
> -  dr_analyze_innermost (dr, nest);
> +  dr_analyze_innermost (dr, nest != NULL ? loop : NULL);
>
> I wonder if it's possible for caller to pass non-NULL nest when
> create_data_ref for a statement in outermost dummy loop?  Or should
> it?

Yeah, it (intentionally) doesn't change whether in_loop is true for
calls via create_data_ref.  But the interface is useful for direct
calls to dr_analyze_innermost.  Currently that's just predcom,
but the later patch adds another in the vectoriser.

> Also noticed that in vect_analyze_data_refs, there is below code:
>
>       /* If target supports vector gather loads or scatter stores, or if
>          this might be a SIMD lane access, see if they can't be used.  */
>       if (is_a <loop_vec_info> (vinfo)
>           && (maybe_gather || maybe_scatter || maybe_simd_lane_access)
>           && !nested_in_vect_loop_p (loop, stmt))
>         {
>           struct data_reference *newdr
>         = create_data_ref (NULL, loop_containing_stmt (stmt),
>                    DR_REF (dr), stmt, maybe_scatter ? false : true);
>
> we have nest==NULL and loop is the inner loop, but after change NULL
> is passed to dr_analyze_innermost because of:
>
> -  dr_analyze_innermost (dr, nest);
> +  dr_analyze_innermost (dr, nest != NULL ? loop : NULL);
>
> Is this the reason that bb-slp-pr65935.c gets vectorized after change?
>  simple_iv should return {&s->phase[dir], 0} successfully though.

No, bb-slp-pr65935.c is testing BB SLP, so we wouldn't have a
loop_vec_info.

> Question is what would happen if simple_iv succeeds with non-ZERO step
> when called with nest==NULL?  The patch skips simple_iv and forces
> ZERO step?

Yeah, I mentioned that in the covering note:

  The handling seemed strange though.  If the DR was part of a loop,
  we still tried to express the base and offset values as IVs, potentially
  giving a nonzero step.  If that failed for any reason, we'd revert to
  using the original base and offset, just as we would if we hadn't asked
  for an IV in the first place.

And like I say, the patch that introduced that behaviour didn't seem
to rely on it.

Thanks,
Richard