On 12/11/2021 13:12, Richard Biener wrote:
> On Thu, 11 Nov 2021, Andre Vieira (lists) wrote:
>
>> Hi,
>>
>> This is the rebased and reworked version of the unroll patch.  I wasn't
>> entirely sure whether I should compare the costs of the unrolled loop_vinfo
>> with the original loop_vinfo it was unrolled of. I did now, but I wasn't too
>> sure whether it was a good idea to... Any thoughts on this?
> +  /* Apply the suggested unrolling factor, this was determined by the
> backend
> +     during finish_cost the first time we ran the analyzis for this
> +     vector mode.  */
> +  if (loop_vinfo->suggested_unroll_factor > 1)
> +    {
> +      poly_uint64 unrolled_vf
> +       = LOOP_VINFO_VECT_FACTOR (loop_vinfo) *
> loop_vinfo->suggested_unroll_factor;
> +      /* Make sure the unrolled vectorization factor is less than the max
> +         vectorization factor.  */
> +      unsigned HOST_WIDE_INT max_vf = LOOP_VINFO_MAX_VECT_FACTOR
> (loop_vinfo);
> +      if (max_vf == MAX_VECTORIZATION_FACTOR || known_le (unrolled_vf,
> max_vf))
> +       LOOP_VINFO_VECT_FACTOR (loop_vinfo) = unrolled_vf;
> +      else
> +       return opt_result::failure_at (vect_location,
> +                                      "unrolling failed: unrolled"
> +                                      " vectorization factor larger than"
> +                                      " maximum vectorization factor:
> %d\n",
> +                                      LOOP_VINFO_MAX_VECT_FACTOR
> (loop_vinfo));
> +    }
> +
>     /* This is the point where we can re-start analysis with SLP forced
> off.  */
>   start_over:
>
> So we're honoring suggested_unroll_factor here but you still have the
> now unused hunk
>
> +vect_analyze_loop_2 (loop_vec_info loop_vinfo, bool &fatal,
> +                    unsigned *suggested_unroll_factor, poly_uint64 min_vf
> = 2)
>   {
>
> I also wonder whether vect_analyze_loop_2 could at least prune
> suggested_unroll_factor as set by vect_analyze_loop_costing with its
> knowledge of max_vf itself?  That would avoid using the at the moment
> unused LOOP_VINFO_MAX_VECT_FACTOR?
>
> I think all the things you do in vect_can_unroll should be figured
> out with the re-analysis, and I'd just amend vect_analyze_loop_1
> with a suggested unroll factor parameter like it has main_loop_vinfo
> for the epilogue case.  The main loop adjustment would the be in the
>
>            if (first_loop_vinfo == NULL)
>              {
>                first_loop_vinfo = loop_vinfo;
>                first_loop_i = loop_vinfo_i;
>                first_loop_next_i = mode_i;
>              }
Sounds good.
>
> spot only, adding
>
> if (loop_vinfo->suggested_unroll_factor != 1)
>    {
>      suggested_unroll_factor = loop_vinfo->suggested_unroll_factor;
>      mode_i = first_loop_i;
>      if (dump)
>        dump_print ("Trying unrolling by %d\n");
>      continue;
>    }
Not quite like this because of how we need to keep the suggestion given 
at finish_costs, put into suggested_unroll_factor, separate from how we 
tell vect_analyze_loop_1 that we are now vectorizing an unrolled vector 
loop, which we do by writing to loop_vinfo->suggested_unroll_factor. 
Perhaps I should renamed the latter, to avoid confusion? Let me know if 
you think that would help and in the mean-time this is what the patch 
looks like now. I'll follow up with a ChangeLog when we settle on the 
name & structure.