[PATCH] D155743: [AggressiveInstCombine] Fold strcmp for short string literals with size 2

[Maksim Kita via Phabricator via llvm-commits]

kitaisreal marked 3 inline comments as done.
kitaisreal added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:997
+                                                  CharacterIndexToCheck)),
+        RetType);
+    Value *ConstantStrCharacterValue = ConstantInt::get(
----------------
goldstein.w.n wrote:
> Since a precondition is the `strcmp` is only used for equality, why do you `zext` here.
> 
Above we check that `strcmp` is only used in any comparison with zero, not just equality comparison.
`zest` is required because in `strcmp` we compare characters as unsigned characters.
Please check discussion in previous pull request which provides more details https://reviews.llvm.org/D154725 file AggressiveInstCombine.cpp line 949.



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:1002
+    Value *CharacterSub =
+        B.CreateNSWSub(StrCharacterValue, ConstantStrCharacterValue);
+    Value *IsCharacterSubZero =
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > is NSW right?
> Likewise, do you need the sub at all? Can you just br on the compare of the two characters and return the compare result itself?
We need `sub` because we want to support not only equality compare `strcmp` result with zero `strcmp(P, 'a') == 0`, but also `strcmp(P, 'a') < 0` and `strcmp(P, 'a') > 0`.
It seems that it can be a good idea to simplify `strcmp(P, 'a') == 0` to something like P[0] == 'a' && P[1] == '\0'. It seems that compiler cannot do this right now.
NSW added to follow standard `strcmp` implementation from glibc https://godbolt.org/z/Pd9xKcnE8.


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

https://reviews.llvm.org/D155743