Re: [PATCH] fix PR46029: reimplement if conversion of loads and stores [2nd submitted version of patch]

[Alan Lawrence <alan.lawrence@arm.com>]

Thanks, Abe. A couple comments below...

> @@ -883,7 +733,7 @@ if_convertible_gimple_assign_stmt_p (gimple stmt,
> 
>     if (flag_tree_loop_if_convert_stores)
>       {
> -      if (ifcvt_could_trap_p (stmt, refs))
> +      if (ifcvt_could_trap_p (stmt))
>         {
>           if (ifcvt_can_use_mask_load_store (stmt))
>             {
> @@ -892,9 +742,17 @@ if_convertible_gimple_assign_stmt_p (gimple stmt,
>               return true;
>             }
>           if (dump_file && (dump_flags & TDF_DETAILS))
> -           fprintf (dump_file, "tree could trap...\n");
> +           fprintf (dump_file, "tree could trap\n");
>           return false;
>         }
> +
> +      if (has_non_addressable_refs (stmt))
> +       {
> +         if (dump_file && (dump_flags & TDF_DETAILS))
> +           fprintf (dump_file, "has non-addressable memory references\n");
> +         return false;
> +       }
> +
>         return true;
>       }

As before, I'm still confused here. This still returns false, i.e. bails out of
if-conversion, if the statement could trap. Doesn't the scratchpad let us handle
that? Or do we just not care because it won't be vectorizable anyway???

> @@ -1342,7 +1190,7 @@ if_convertible_loop_p_1 (struct loop *loop,
>         /* Check the if-convertibility of statements in predicated BBs.  */
>         if (!dominated_by_p (CDI_DOMINATORS, loop->latch, bb))
>         for (itr = gsi_start_bb (bb); !gsi_end_p (itr); gsi_next (&itr))
> -         if (!if_convertible_stmt_p (gsi_stmt (itr), *refs,
> +         if (!if_convertible_stmt_p (gsi_stmt (itr),
>                                       any_mask_load_store))

Nit: as before - line no longer needs wrapping (a few other cases too)

> @@ -2063,12 +1997,14 @@ mask_exists (int size, vec<int> vec)
>      | end_bb_1
>      |
>      | bb_2
> +   |   cond = some_computation;

Nit: as before - thanks for fixing the example here, but...

>      |   if (cond) goto bb_3 else goto bb_4
>      | end_bb_2
>      |
>      | bb_3
>      |   cond = some_computation;

...I think you mean to remove this last too.

> @@ -2817,10 +2761,26 @@ public:
>   bool
>   pass_if_conversion::gate (function *fun)
>   {
> -  return (((flag_tree_loop_vectorize || fun->has_force_vectorize_loops)
> -          && flag_tree_loop_if_convert != 0)
> -         || flag_tree_loop_if_convert == 1
> -         || flag_tree_loop_if_convert_stores == 1);
> +  return  (
> +            (
> +              flag_tree_loop_vectorize
> +           || fun->has_force_vectorize_loops
> +            )
> +         && (
> +              (
> +                flag_tree_loop_if_convert        != 0
> +              )
> +           || (
> +                flag_tree_loop_if_convert_stores != 0
> +              )
> +            )
> +         )
> +         || (
> +              flag_tree_loop_if_convert        > 0
> +            )
> +         || (
> +              flag_tree_loop_if_convert_stores > 0
> +            );
>   }

That is quite complex. Where can I find info on what the different flag values 
mean? (I had thought they were booleans, clearly I'm wrong, but a quick scan 
through invoke.texi doesn't seem to help; both your testcases and your updates 
to invoke.texi say e.g. -ftree-loop-if-convert-stores not 
-ftree-loop-if-convert-stores=<value>)


Cheers, Alan