[llvm-commits] [llvm] r122959 - in /llvm/trunk: lib/Target/README.txt lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/objsize.ll

[Benjamin Kramer]

On 07.01.2011, at 21:00, Bob Wilson wrote:

> On Jan 6, 2011, at 5:11 AM, Benjamin Kramer wrote:
> 
>> Author: d0k
>> Date: Thu Jan  6 07:11:05 2011
>> New Revision: 122959
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=122959&view=rev
>> Log:
>> InstCombine: If we call llvm.objectsize on a malloc call we can replace it with the size passed to malloc.
>> 
>> Modified:
>>   llvm/trunk/lib/Target/README.txt
>>   llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>   llvm/trunk/test/Transforms/InstCombine/objsize.ll
> 
> This caused a significant performance regression in the MultiSource/Benchmarks/Prolangs-C/cdecl test:
> http://llvm.org/perf/db_default/simple/nts/69/
> 
> The compile-time regression is just a symptom of having larger code.  The ds() function in cdecl.c is:
> 
> char *ds(s)
> char *s;
> {
>    register char *p = malloc((unsigned)(strlen(s)+1));
> 
>    if (p)
> 	(void) strcpy(p,s);
>    else
> 	{
> 	(void) fprintf (stderr, "%s: malloc() failed!\n", progname);
> 	exit(1);
> 	}
>    return p;
> }
> 
> Before this change, codegenprepare folded the strcpy_chk to strcpy because it treated the malloc size as unknown.  Now, it knows the malloc size but not the source size, so it's keeping the strcpy_chk.  The ds() function is inlined to lots of places, so the effect is greatly magnified.  In this case, it ought to be possible to recognize that the source is the same size as the destination and have instcombine fold to strcpy.  Could you look into doing that, or at least add a comment to the README file about it?

I reverted the change for now, there is another issue with this as it changes the semantics of __builtin_objectsize.
Unless we want to extend this GCC extension even more my patch is not safe in it's current form :(