[InstCombine] Replace bcmp/bcopy with memcmp/memmove

[xbolva00]

Summary

bcmp(s1, s2, n) -> memcmp(s1, s2, n)
bcopy(src, dst, n) -> memmove(dst, src, n)

[bkramer]

why?

[xbolva00]

In D47214#1108622, @bkramer wrote:

why?

Anyway these functions call mem* functions, so why not call them directly?

[sanjoy]

In D47214#1108641, @xbolva00 wrote:

In D47214#1108622, @bkramer wrote:

why?

Anyway these functions call mem* functions, so why not call them directly?

There are an infinite number of optimizations we can implement, but each one adds more maintenance burden. We need to make sure new optimizations "carry their weight".

Do you have example programs or benchmarks that measurably speed up or have smaller code size etc. because of this transform?

[xbolva00]

In D47214#1108642, @sanjoy wrote:

In D47214#1108641, @xbolva00 wrote:

In D47214#1108622, @bkramer wrote:

why?

Anyway these functions call mem* functions, so why not call them directly?

There are an infinite number of optimizations we can implement, but each one adds more maintenance burden. We need to make sure new optimizations "carry their weight".

Do you have example programs or benchmarks that measurably speed up or have smaller code size etc. because of this transform?

I have no benchmarks. But changing them to mem-functions allows us to apply another set of transformations in "optimizeMemcmp/Memmove".

Anyway, If you say no, no problem. I will close this patch.

[sanjoy]

In D47214#1108647, @xbolva00 wrote:

I have no benchmarks. But changing them to mem-functions allows us to apply another set of transformations in "optimizeMemcmp/Memmove".

I see. Are those secondary transformations important for some workload?

Anyway, If you say no, no problem. I will close this patch.

To be clear, I don't want to discourage you from contributing to LLVM. It is just that developer time (your time, reviewer time etc.) is expensive and we should be spending that towards the kinds of optimizations that matter, not towards implementing every possible transform under the sun.

[xbolva00]

Well, this patch was not so time expensive for me, it is simple. But ok, I understand your concerns related to maintainability. So, should I close this review? Or ok for now, since it is already done?

[xbolva00]

And two facts..

1. LLVM already replaces bzero with memset
2. There are many source codes related to networking which uses bcopy, bcmp.

So I dont think this patch is totally useless.

Maybe @Prazek can do review of this patch for me?

[xbolva00]

ping

[Prazek]

In D47214#1109007, @xbolva00 wrote:

And two facts..

1. LLVM already replaces bzero with memset
2. There are many source codes related to networking which uses bcopy, bcmp.

So I dont think this patch is totally useless.

Maybe @Prazek can do review of this patch for me?

The code and tests look good, but I have the same opinion as others - we can't add new transforms without them providing a sufficient value compared to maintaining cost.
Maybe try to come up with at least a microbenchmark that would show the gain. You can also benchmark LNT test suite and see if there is any improvement, but the microbenchmark should be easier.

[sanjoy]

In D47214#1114639, @Prazek wrote:

Maybe try to come up with at least a microbenchmark that would show the gain. You can also benchmark LNT test suite and see if there is any improvement, but the microbenchmark should be easier.

Microbenchmarking is easier (almost every optimization can be shown to be "profitable" with the right microbenchmark :) ), but you'll have an easier time convincing us if there is an improvement in LNT (or any other "standard" benchmark really, like SPEC).

[xbolva00]

Okey, abandoning this patch since these functions are deprecated away and doing SPEC two times just to prove something.. No, it is a waste of time.

include/llvm/Transforms/Utils/SimplifyLibCalls.h

private:
   Value *optimizeStrStr(CallInst *CI, IRBuilder<> &B);
   Value *optimizeMemChr(CallInst *CI, IRBuilder<> &B);
   Value *optimizeMemCmp(CallInst *CI, IRBuilder<> &B);
   Value *optimizeMemCpy(CallInst *CI, IRBuilder<> &B);
   Value *optimizeMemMove(CallInst *CI, IRBuilder<> &B);
   Value *optimizeMemSet(CallInst *CI, IRBuilder<> &B);
   Value *optimizeRealloc(CallInst *CI, IRBuilder<> &B);
   Value *optimizeWcslen(CallInst *CI, IRBuilder<> &B);
+  Value *optimizeBCmp(CallInst *CI, IRBuilder<> &B);
+  Value *optimizeBCopy(CallInst *CI, IRBuilder<> &B);
   // Wrapper for all String/Memory Library Call Optimizations
   Value *optimizeStringMemoryLibCall(CallInst *CI, IRBuilder<> &B);
 
   // Math Library Optimizations
   Value *optimizeCAbs(CallInst *CI, IRBuilder<> &B);
   Value *optimizeCos(CallInst *CI, IRBuilder<> &B);
   Value *optimizePow(CallInst *CI, IRBuilder<> &B);
   Value *replacePowWithSqrt(CallInst *Pow, IRBuilder<> &B);

lib/Transforms/Utils/SimplifyLibCalls.cpp

if (Str.empty() && CI->use_empty()) {
     if (CI->use_empty() || !Res)
       return Res;
     return B.CreateIntCast(Res, CI->getType(), true);
   }
 
   return nullptr;
 }
 
+Value *LibCallSimplifier::optimizeBCmp(CallInst *CI, IRBuilder<> &B) {
+  // bcmp(s1, s2, n) -> memcmp(s1, s2, n)
+  return emitMemCmp(CI->getOperand(0), CI->getOperand(1), CI->getOperand(2), B,
+                    DL, TLI);
+}
+
+Value *LibCallSimplifier::optimizeBCopy(CallInst *CI, IRBuilder<> &B) {
+  // bcopy(src, dst, n) -> memmove(dst, src, n)
+  return B.CreateMemMove(CI->getArgOperand(1), 1, CI->getArgOperand(0), 1,
+                         CI->getArgOperand(2));
+}
+
 bool LibCallSimplifier::hasFloatVersion(StringRef FuncName) {
   LibFunc Func;
   SmallString<20> FloatFuncName = FuncName;
   FloatFuncName += 'f';
   if (TLI->getLibFunc(FloatFuncName, Func))
     return TLI->has(Func);
   return false;
 }
if (TLI->getLibFunc(*Callee, Func) && TLI->has(Func)) {
     case LibFunc_memmove:
       return optimizeMemMove(CI, Builder);
     case LibFunc_memset:
       return optimizeMemSet(CI, Builder);
     case LibFunc_realloc:
       return optimizeRealloc(CI, Builder);
     case LibFunc_wcslen:
       return optimizeWcslen(CI, Builder);
+    case LibFunc_bcmp:
+      return optimizeBCmp(CI, Builder);
+    case LibFunc_bcopy:
+      return optimizeBCopy(CI, Builder);
     default:
       break;
     }
   }
   return nullptr;
 }
 
 Value *LibCallSimplifier::optimizeFloatingPointLibCall(CallInst *CI,

test/Transforms/InstCombine/bcmp-bcopy.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
+
+declare i32 @bcmp(i8* nocapture, i8* nocapture, i32)
+declare void @bcopy(i8* nocapture readonly, i8* nocapture, i32)
+
+define i32 @bcmp_memcmp(i8* nocapture readonly %a, i8* nocapture readonly %b, i32 %len) {
+; CHECK-LABEL: @bcmp_memcmp(
+; CHECK-NEXT:    [[MEMCMP:%.*]] = call i32 @memcmp(i8* [[A:%.*]], i8* [[B:%.*]], i32 [[LEN:%.*]])
+; CHECK-NEXT:    ret i32 [[MEMCMP]]
+;
+  %call = tail call i32 @bcmp(i8* %a, i8* %b, i32 %len) #4
+  ret i32 %call
+}
+
+define void @bcopy_memmove(i8* nocapture readonly %a, i8* nocapture %b, i32 %len) {
+; CHECK-LABEL: @bcopy_memmove(
+; CHECK-NEXT:    call void @llvm.memmove.p0i8.p0i8.i32(i8* align 1 [[B:%.*]], i8* align 1 [[A:%.*]], i32 [[LEN:%.*]], i1 false)
+; CHECK-NEXT:    ret void
+;
+  tail call void @bcopy(i8* %a, i8* %b, i32 %len) #5
+  ret void
+}
+