Re: [PATCH] PR 78534 Change character length from int to size_t

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

On Wed, Dec 21, 2016 at 3:14 PM, Andre Vehreschild <vehre@gmx.de> wrote:
>> Now when I think about this some more, I have a vague recollection
>> that a long time ago it used to be something like that.  The problem
>> is that MIN_EXPR<CONSTANT, NON-CONSTANT> will of course be
>> NON-CONSTANT, so the memcpy call can't be inlined. Hence it was
>> changed to two separate __builtin_memmove() calls to have better
>> opportunity to inline. So probably a no-go to change it back. :(
>
> I don't get that. From the former only one of the memmove's could have been
> inlined assuming that only CONSTANT sizes are inlined.

Yes. Which is better than not being able to inline, assuming the
inlined branch is more likely to be taken, no?

> The other one had a
> NON-CONSTANT as long as pointer following and constant propagation was not
> effective together. In our case the "NON-CONSTANT branch" would have been used,
> which is resolved by constant propagation (of the size of constant memory p
> points to). I assume the warning is triggered, because dead-code elimination
> has not removed the else part.

Probably yes, in this case. My performance worries were more about the
general case. But perhaps they are unfounded, IDK really. Does anybody
have a battery of string operation benchmarks that mirrors how real
Fortran code does string handling?

Anyway, the attached patch accomplishes that. It turns the tree dump I
showed two days ago into something like

  {
    integer(kind=8) D.3484;
    unsigned long D.3485;

    D.3484 = *_p;
    D.3485 = (unsigned long) D.3484 + 18446744073709551608;
    if (D.3484 != 0)
      {
        __builtin_memmove ((void *) *p, (void *) &"12345679"[1]{lb: 1
sz: 1}, MIN_EXPR <(unsigned long) D.3484, 8>);
        if ((unsigned long) D.3484 > 8)
          {
            __builtin_memset ((void *) *p + 8, 32, D.3485);
          }
      }
  }

and gets rid of the -Wstringop-overflow warning. (It causes a two new
testsuite failures (gfortran.dg/dependency_49.f90 and
gfortran.dg/transfer_intrinsic_1.f90), but those are just tests that
grep for patterns in the .original dump and need to be adjusted).

If that is Ok, do you prefer it as part of the charlen-size_t patch,
or would you (GFortran maintainers in general) prefer that it's a
separate patch?

-- 
Janne Blomqvist

Attachment: stringcopy.diff
Description: Text document