[PATCH] D154725: [AggressiveInstCombine] Fold strcmp for short string literals

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:949
+  Value *Str1FirstCharacterValue =
+      B.CreateIntCast(B.CreateLoad(B.getInt8Ty(), Str1P), B.getInt32Ty(), true);
+  Value *Str2FirstCharacterValue = ConstantInt::get(
----------------
kitaisreal wrote:
> nikic wrote:
> > Why the use of sign extension? The C standard says the comparison is performed on unsigned char, not signed char:
> > 
> > > The sign of a nonzero value returned by the comparison functions memcmp, strcmp, and strncmp
> > is determined by the sign of the difference between the values of the first pair of characters (both
> > interpreted as unsigned char) that differ in the objects being compared.
> I cast this `char` to `int32` so I can later sub them as signed integers. This looks simillar to standard glibc implementation https://github.com/zerovm/glibc/blob/master/string/strcmp.c#L45C15-L45C15. If we take this standard implementation and look and the IR it will be similar to mine https://godbolt.org/z/Pd9xKcnE8.
You can do the subtraction as you do now, but the characters need to be zero-extended. Otherwise you will claim that `'\x7f'` is larger than `'\x80'`, which is not correct according to strcmp semantics.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:992
+  if (!IsCallingConvC)
+    return false;
+
----------------
kitaisreal wrote:
> nikic wrote:
> > Why is this relevant?
> I take it from `SimplifyLibCalls.cpp`. Not sure if this is safe to ignore it for our transformation.
> ```
> // Then check for known library functions.
> if (TLI->getLibFunc(*Callee, Func) && isLibFuncEmittable(M, TLI, Func)) {
>   // We never change the calling convention.
>   if (!ignoreCallingConv(Func) && !IsCallingConvC)
>     return nullptr;
>   if (Value *V = optimizeStringMemoryLibCall(CI, Builder))
>     return V;
> ```
We're removing the call entirely, so calling convention doesn't matter.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:1003
+  if (CI->isMustTailCall() || CI->isNoTailCall())
+    return false;
+
----------------
kitaisreal wrote:
> nikic wrote:
> > Why is this relevant?
> I take it from `InstCombineCalls.cpp`. Not sure if this is safe to ignore it for our transformation.
> ```
> // Skip optimizing notail and musttail calls so
> // LibCallSimplifier::optimizeCall doesn't have to preserve those invariants.
> // LibCallSimplifier::optimizeCall should try to preseve tail calls though.
> if (CI->isMustTailCall() || CI->isNoTailCall())
>   return nullptr;
> ```
Same here. This doesn't matter as the call is removed entirely.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:959
+  Value *Str2FirstCharacterValue = ConstantInt::get(
+      B.getInt32Ty(), static_cast<unsigned char>(ConstantStr[0]), true);
+  Value *FirstCharacterSub =
----------------
Rather than hardcoding `B.getInt32Ty()`, it is better to use the original type (`CI->getType()`). There are platforms where `int` is not 32-bit.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/strcmp.ll:25
 
 define i1 @expand_strcmp_s1_1(ptr %C) {
 ; CHECK-LABEL: @expand_strcmp_s1_1(
----------------
`@expand_strcmp_eq_s1`


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/strcmp.ll:47
 
 define i1 @expand_strcmp_s1_2(ptr %C) {
 ; CHECK-LABEL: @expand_strcmp_s1_2(
----------------
`@expand_strcmp_eq_s1_commuted`


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/strcmp.ll:69
 
 define i1 @expand_strcmp_s1_3(ptr %C) {
 ; CHECK-LABEL: @expand_strcmp_s1_3(
----------------
`@expand_strcmp_ne_s1`

And so on, to get at least a slightly meaningful test name, rather than just a counter.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/strcmp.ll:108
 ;
   %call = call i32 @strcmp(ptr noundef @s1, ptr %C)
   %cmp = icmp ne i32 %call, 0
----------------
It's okay to have just one test with the constant on the left-hand-side. No need to test LHS and RHS for all predicates.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154725/new/

https://reviews.llvm.org/D154725