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

[Andre Vehreschild <vehre at gmx dot de>]

Hi Janne,

sorry for being late in voicing my opinion, but I personally would prefer to
have this patch in a separately. That way bisecting for performance
regressions points only to the offending code and not to the change of the
character length (the latter might add a tiny bit of cost, too).

Regards,
	Andre


On Fri, 23 Dec 2016 11:27:10 +0200
Janne Blomqvist <blomqvist.janne@gmail.com> wrote:

> 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?
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de