Another fix for decide_alg (PR target/70062)

On Thu, Mar 3, 2016 at 9:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Before my recent decide_alg change, *dynamic_check == -1 was indeed
> guaranteed, because any_alg_usable_p doesn't depend on the arguments of
> decide_alg that might change during recursive call, so we'd only recurse if
> it wouldn't set *dynamic_check.  But, if we give up because we'd otherwise
> recurse infinitely, we can set *dynamic_check to 128.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
>
> 2016-03-03  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/70062
>         * config/i386/i386.c (decide_alg): If
>         TARGET_INLINE_STRINGOPS_DYNAMICALLY, allow *dynamic_check to be also
>         128 from the recursive call.
>
>         * gcc.target/i386/pr70062.c: New test.

I don't like the fact that *dynamic_check is set to max (which is 0
with your testcase) when recursion avoidance code already set it to
"something reasonable", together with loop_1_byte alg. What do you
think about attached (lightly tested) patch?

Uros.

On Fri, Mar 04, 2016 at 10:59:48AM +0100, Uros Bizjak wrote:
> I don't like the fact that *dynamic_check is set to max (which is 0
> with your testcase) when recursion avoidance code already set it to
> "something reasonable", together with loop_1_byte alg. What do you
> think about attached (lightly tested) patch?

But that can still set *dynamic_check to 0 if the recursive call has
not set *dynamic_check.
So, perhaps we want *dynamic_check = max ? max : 128;
?

> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 8a026ae..ded9951 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -26170,11 +26170,22 @@ decide_alg (HOST_WIDE_INT count, HOST_WIDE_INT expected_size,
>          }
>        alg = decide_alg (count, new_expected_size, min_size, max_size, memset,
>  			zero_memset, have_as, dynamic_check, noalign);
> -      gcc_assert (*dynamic_check == -1);
> +
>        if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
> -	*dynamic_check = max;
> +	{
> +	  /* *dynamic_check could be set to 128 above because we avoided
> +	     infinite recursion.  */
> +	  if (*dynamic_check == 128)
> +	    gcc_assert (alg == loop_1_byte);
> +	  else
> +	    {
> +	      gcc_assert (*dynamic_check == -1);
> +	      *dynamic_check = max;
> +	    }
> +	}
>        else
> -	gcc_assert (alg != libcall);
> +	gcc_assert (alg != libcall && *dynamic_check == -1);
> +
>        return alg;
>      }
>    return (alg_usable_p (algs->unknown_size, memset, have_as)


	Jakub

On Fri, Mar 4, 2016 at 11:06 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Mar 04, 2016 at 10:59:48AM +0100, Uros Bizjak wrote:
>> I don't like the fact that *dynamic_check is set to max (which is 0
>> with your testcase) when recursion avoidance code already set it to
>> "something reasonable", together with loop_1_byte alg. What do you
>> think about attached (lightly tested) patch?
>
> But that can still set *dynamic_check to 0 if the recursive call has
> not set *dynamic_check.
> So, perhaps we want *dynamic_check = max ? max : 128;
> ?

I believe that recursive call set *dynamic_check to zero for a reason.
The sent patch deals with recursion breaking issues only, leaving all
other functionality as it was before. So, your issue is IMO orthogonal
to the PR70062 and should be fixed (if at all) in a separate patch.

Uros.

>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index 8a026ae..ded9951 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -26170,11 +26170,22 @@ decide_alg (HOST_WIDE_INT count, HOST_WIDE_INT expected_size,
>>          }
>>        alg = decide_alg (count, new_expected_size, min_size, max_size, memset,
>>                       zero_memset, have_as, dynamic_check, noalign);
>> -      gcc_assert (*dynamic_check == -1);
>> +
>>        if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
>> -     *dynamic_check = max;
>> +     {
>> +       /* *dynamic_check could be set to 128 above because we avoided
>> +          infinite recursion.  */
>> +       if (*dynamic_check == 128)
>> +         gcc_assert (alg == loop_1_byte);
>> +       else
>> +         {
>> +           gcc_assert (*dynamic_check == -1);
>> +           *dynamic_check = max;
>> +         }
>> +     }
>>        else
>> -     gcc_assert (alg != libcall);
>> +     gcc_assert (alg != libcall && *dynamic_check == -1);
>> +
>>        return alg;
>>      }
>>    return (alg_usable_p (algs->unknown_size, memset, have_as)
>
>
>         Jakub

On Fri, Mar 04, 2016 at 11:16:44AM +0100, Uros Bizjak wrote:
> On Fri, Mar 4, 2016 at 11:06 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Mar 04, 2016 at 10:59:48AM +0100, Uros Bizjak wrote:
> >> I don't like the fact that *dynamic_check is set to max (which is 0
> >> with your testcase) when recursion avoidance code already set it to
> >> "something reasonable", together with loop_1_byte alg. What do you
> >> think about attached (lightly tested) patch?
> >
> > But that can still set *dynamic_check to 0 if the recursive call has
> > not set *dynamic_check.
> > So, perhaps we want *dynamic_check = max ? max : 128;
> > ?
> 
> I believe that recursive call set *dynamic_check to zero for a reason.
> The sent patch deals with recursion breaking issues only, leaving all
> other functionality as it was before. So, your issue is IMO orthogonal
> to the PR70062 and should be fixed (if at all) in a separate patch.

The recursive call should never set *dynamic_check to 0, only to
-1 or 128 (the last one newly, since my fix).
And, my issue is IMO not orghogonal to that, either *dynamic_check == 0
is ok, or it is not.
Perhaps better would be to add an extra argument to decide_alg, false
for toplevel invocation, true for recursive invocation, and if recursive
call, just never try to recurse again (thus we could revert the previous
change) and don't set *dynamic_check to anything in that case during the
recursion.
And then at the caller side decide what we want for max == -1 and what
for max == 0 with *dynamic_check.

	Jakub

On Fri, Mar 4, 2016 at 11:34 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Mar 04, 2016 at 11:16:44AM +0100, Uros Bizjak wrote:
>> On Fri, Mar 4, 2016 at 11:06 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Fri, Mar 04, 2016 at 10:59:48AM +0100, Uros Bizjak wrote:
>> >> I don't like the fact that *dynamic_check is set to max (which is 0
>> >> with your testcase) when recursion avoidance code already set it to
>> >> "something reasonable", together with loop_1_byte alg. What do you
>> >> think about attached (lightly tested) patch?
>> >
>> > But that can still set *dynamic_check to 0 if the recursive call has
>> > not set *dynamic_check.
>> > So, perhaps we want *dynamic_check = max ? max : 128;
>> > ?
>>
>> I believe that recursive call set *dynamic_check to zero for a reason.
>> The sent patch deals with recursion breaking issues only, leaving all
>> other functionality as it was before. So, your issue is IMO orthogonal
>> to the PR70062 and should be fixed (if at all) in a separate patch.
>
> The recursive call should never set *dynamic_check to 0, only to
> -1 or 128 (the last one newly, since my fix).
> And, my issue is IMO not orghogonal to that, either *dynamic_check == 0
> is ok, or it is not.
> Perhaps better would be to add an extra argument to decide_alg, false
> for toplevel invocation, true for recursive invocation, and if recursive
> call, just never try to recurse again (thus we could revert the previous
> change) and don't set *dynamic_check to anything in that case during the
> recursion.
> And then at the caller side decide what we want for max == -1 and what
> for max == 0 with *dynamic_check.

I think that would work, and considering that we have only one
non-recursive callsite of decide_alg, it looks like the cleanest
solution.

Uros.