Page MenuHomePhabricator

[InstCombine] Replace bcmp/bcopy with memcmp/memmove
AbandonedPublic

Authored by xbolva00 on May 22 2018, 12:04 PM.

Details

Summary

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

Diff Detail

Event Timeline

xbolva00 created this revision.May 22 2018, 12:04 PM
xbolva00 added a comment.EditedMay 22 2018, 2:01 PM

why?

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

sanjoy added a subscriber: sanjoy.May 22 2018, 2:06 PM

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?

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.

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 added a comment.EditedMay 22 2018, 2:33 PM

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 added a comment.EditedMay 22 2018, 8:28 PM

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?

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.

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 abandoned this revision.May 31 2018, 9:53 AM

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.