[PATCH] D154725: [AggressiveInstCombine] Fold strcmp for short string literals
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 13 13:31:04 PDT 2023
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
More information about the llvm-commits
mailing list