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

[Maksim Kita via Phabricator via llvm-commits]

kitaisreal marked 7 inline comments as done.
kitaisreal 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(
----------------
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.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:992
+  if (!IsCallingConvC)
+    return false;
+
----------------
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;
```


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:1003
+  if (CI->isMustTailCall() || CI->isNoTailCall())
+    return false;
+
----------------
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;
```


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

https://reviews.llvm.org/D154725