Re: [PATCH 5/7 v6] vect: Support vector load/store with length in vectorizer

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Richard,

on 2020/7/7 下午6:15, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2020/7/1 下午11:17, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>> on 2020/7/1 上午3:53, Richard Sandiford wrote:
>>>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>>
>>>> Sorry, I didn't quite follow this comment.  Both nscalars_totoal and
>>>> nscalars_step are scaled here.  The remaining related nscalars_*
>>>> seems only nscalars_skip, but length can't support skip.
>>>
>>> Hmm, OK.  But in that case can you update the names of the variables
>>> to match?  It's confusing to have some nscalars_* variables actually
>>> count scalars (and thus have “nitems” equivalents) and other nscalars_*
>>> variables count something else (and thus effectively be nitems_* variables
>>> themselves).
>>>
>>
>> OK.  I'll update the names like nscalars_total/nscalars_step and equivalents
>> to nitems_total/... (or nunits_total better?)
> 
> I agree “items” isn't great.  I was trying to avoid “units” because GCC
> often uses that to mean bytes (BITS_PER_UNIT, UNITS_PER_WORD, etc.).
> In this context that could be confusing, because sometimes the
> “units” actually would be bytes, but not always.
> 

Got it!  Thanks!

[...]
>>> But in this particular test, we're doing outer loop vectorisation,
>>> and the only elements of vres that matter are the ones selected
>>> by loop_mask (since those are the only ones that get stored out).
>>> So applying the loop mask to the VEC_COND_EXPR is “just” an
>>> (important) optimisation, rather than a correctness issue.
>>>  
>>
>> Thanks for the clarification.  It looks the vres is always safe since its
>> further usage is guard with loop mask.  Then sorry that I didn't catch why
>> it is one optimization for this case, is there some difference in backend
>> supports on this different mask for cond_expr?
> 
> No, the idea of the optimisation is to avoid cases in which we have:
> 
>     cmp_res = …compare…
>     cmp_res' = cmp_res & loop_mask
>     IFN_MASK_LOAD (…, cmp_res')
>     z = cmp_res ? x : y
> 
> The problem here is that cmp_res and cmp_res' are live at the same time,
> which prevents cmp_res and cmp_res' from being combined into a single
> instruction.  It's better for the final instruction to be:
> 
>     z = cmp_res' ? x : y
> 
> so that everything uses the same comparison result.
> 
> We can't leave that to later passes because nothing in the gimple IL
> indicates that only the loop_mask elements of z matter.
> 

Nice, thanks for the explanation.


[...]
>>> What's causing the test to start failing with the patch?  I realise
>>> you've probably already said, sorry, but it's been a large patch series
>>> so it's hard to keep all the details committed to memory.
>>>
>>
>> No problem, appreciate your time much!  Since length-based partial vectors
>> doesn't support any reduction so far, the function has the responsibility
>> to disable use_partial_vectors_p for it.  Without the above else-if part,
>> since the reduction_type is TREE_CODE_REDUCTION for this case, the else part
>> will stop this case to use mask-based partial vectors, but the case expects
>> the outer loop still able to use mask-based partial vectors.
>>
>> As your clarification above, else-if looks wrong.  Probably we can change it
>> to check whether the current vectorization is for outer loop and the condition
>> stmt being handled is in the inner loop, we can allow it for partial vectors?
> 
> I think it's more whether, for outer loop vectorisation, the reduction
> is a double reduction or a simple nested-cycle reduction.  Both have
> a COND_EXPR in the inner loop, but the extra elements only matter for
> double reductions.
> 
> There again, I don't think we actually support double reductions for
> COND_EXPR reductions.
> 

OK.  I will send one separate patch with your suggestion on this.

[...]
>>
>> OK, but it seems it's acceptable if the IV wider than the precision since
>> we allows it out of range?
> 
> For example, suppose that a target handled out-of-range values but
> still had a QImode length.  If the IV was wider than QI, we'd truncate
> 0x100 to 0 when generating the pattern, so a full-vector access would
> get truncated to an empty-vector access.
> 

Yeah, it's so true.

BR,
Kewen