On Tue, Dec 27, 2016 at 12:47 PM, Andre Vehreschild <ve...@gmx.de> wrote:
> 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).
No worries. I included this in the updated charlen-size_t patch I
posted and which FX reviewed, but I can certainly commit this part
separately.
>
> 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
--
Janne Blomqvist
