[llvm-commits] [llvm] r61297 - in /llvm/trunk: lib/Transforms/Scalar/SimplifyLibCalls.cpp test/Transforms/SimplifyLibCalls/2008-12-20-StrcmpMemcmp.ll

[Török Edwin]

On 2008-12-21 03:03, Nick Lewycky wrote:
> Eli Friedman wrote:
>   
>> On Sat, Dec 20, 2008 at 4:38 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>     
>>> On Sat, Dec 20, 2008 at 4:19 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>>>       
>>>> Author: nicholas
>>>> Date: Sat Dec 20 18:19:21 2008
>>>> New Revision: 61297
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=61297&view=rev
>>>> Log:
>>>> Turn strcmp into memcmp, such as strcmp(P, "x") --> memcmp(P, "x", 2).
>>>>         
>>> I'm pretty sure this isn't safe; take the following testcase:
>>>
>>> int foo(char* x) {return strcmp(x, "x", 2) == 0;}
>>>       
>> Oops, messed that up slightly; try the following:
>> int foo(char* x) {return strcmp(x, "x") == 0;}
>>     
>
> The pathological case is:
>
>    char x[1] = "\0";  // pretend we don't know its length
>    char y[2] = "x\0"; // we know its length
>    return strcmp(x, y);
>
> If we transform it into
>
>    return memcmp(x, y, 2)
>
> then the existing MemCmpOpt will exercise these cases:
>
>    // memcmp(S1,S2,2) != 0 -> (*(short*)LHS ^ *(short*)RHS)  != 0
>    // memcmp(S1,S2,4) != 0 -> (*(int*)LHS ^ *(int*)RHS)  != 0
>
> resulting in a 2-byte load of x[1], which is illegal.
>
> Could someone comment on what part of this transform is wrong? I recall 
> having a discussion about it on IRC, but can't recall the details...
>   


You need to check the alignment.
I think something like:
MinAlignStr = Minimum Alignment of Str1P, Str2P (max 16)
MinLen = Minimum length of Str1P and Str2P
AlignMinLen = AlignOf(MinLen) (max 16)
If (MinAlignStr >= AlingMinLen) -> transform is safe

This assumes that memcmp won't do an 8-byte load if the pointers are
only 4-byte aligned. On IRC most people say this is the case.

If memcpy does do unaligned loads, I think the transform needs an
additional condition: MinAlignStr >= (MinLen % MAX_MEMCPY_LOAD),
where MAX_MEMCPY_LOAD is the maximum memcpy can load in one go,
unaligned, that is usually 8 bytes (because 16-byte SSE loads must be
aligned)

Best regards,
--Edwin