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.ja...@gmail.com> wrote:
> On Wed, Dec 21, 2016 at 3:14 PM, Andre Vehreschild <ve...@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
