[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-23 14:17, Török Edwin wrote:
> On 2008-12-23 02:56, Chris Lattner wrote:
>   
>> On Dec 22, 2008, at 7:15 AM, Török Edwin wrote:
>>   
>>     
>>>>> 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
>>>>
>>>>       
>>>>         
>>> But try this with llvm-gcc:
>>> #include <string.h>
>>> int foo(const char *s)
>>> {
>>>    return !memcmp(s,"x",2);
>>> }
>>>
>>> It produces:
>>> foo:
>>> .Leh_func_begin1:
>>> .Llabel1:
>>>    cmpw    $120, (%rdi)
>>>    sete    %al
>>>    movzbl    %al, %eax
>>>    ret
>>>
>>> So is it a bug in MemCmp optimization?
>>>     
>>>       
>> hi Edwin,
>>
>> This is a really important question that I don't know the answer to.   
>> My understanding is that memcmp only touches the bytes necessary to  
>> make a decision: it is not allowed to touch the full size if  
>> unneeded.  However, I don't really *know* that, and nothing I find  
>> online in a quick scan comes up with an obvious answer.
>>
>> Can you try asking on comp.lang.c or something like that? 
>>     
>
> I found 2 messages on comp.lang.c about this:
> http://groups.google.com/group/comp.lang.c/msg/28fcc4a0e29e5339
> http://groups.google.com/group/comp.lang.c/msg/786c9f1cb2e9b085
>
> "     /* or can probably use memcmp since in practice it will safely
>        stop on or near null != nonnull and return nonmatch, but not
>       AFAICT guaranteed, and being not str* may alarm some */
>
> You seem to be correct - "7.21.4.1 The memcmp function" doesn't require it
> to stop as long as it ultimately returns the correct result - but it's in
> the implementation's interests to make it as efficient as possible."
>
>   
>>  It would  
>> also be interesting to look at the source for various memcmp  
>> implementations 
>>     
>
> glibc:
> /* The strategy of this memcmp is:
>
>    1. Compare bytes until one of the block pointers is aligned.
>
>    2. Compare using memcmp_common_alignment or
>       memcmp_not_common_alignment, regarding the alignment of the other
>       block after the initial byte operations.  The maximum number of
>       full words (of type op_t) are compared in this way.
>
>    3. Compare the few remaining bytes.  */
>
> However in memcmp_not_common_alignment:
> /* memcmp_not_common_alignment -- Compare blocks at SRCP1 and SRCP2 with LEN
>    `op_t' objects (not LEN bytes!).  SRCP2 should be aligned for memory
>    operations on `op_t', but SRCP1 *should be unaligned*.  */
> switch (len % 4) {
> case 2:
>       a1 = ((op_t *) srcp1)[0];
>       a2 = ((op_t *) srcp1)[1];
> ...
> }
>
> It does a 2-byte read on srcp1, which is unaligned, so I think it may
> cross a page boundary if we aren't allowed to do 2-byte reads on all the
> region we pass to memcmp.
>
>
>   
>> to decide if they are safe on commonly available  
>> systems (worst-case this becomes a target-specific optimization).
>>   
>>     
>
> How about using the alignment and length of the string to determine that
> memcmp won't cross a page boundary, even
> with an implementation that does 8-byte reads always?
> Alternatively we may align the operands with some extra code inserted.
>
> I remember seeing valgrind warnings with older versions about strcmp,
> and valgrind has --partial-loads-ok, so
> I guess strcmp already does loads that cross the boundary of one of its
> arguments, if it knows that it is safe to do so (i.e. it won't cross
> page boundaries).

[sorry, hit Send button too soon]

We should also add a testcase to test/ or in test-suite/ that
specifically tests these corner cases.
We can allocate a single page with mmap, so that the OS will kill the
program if it crosses page boundary.

Best regards,
--Edwin