[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 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).

Best regards,
--Edwin