[PATCH] D103009: [DSE] Transform memset + malloc --> calloc (PR25892)

[Alexander Kornienko via Phabricator via llvm-commits]

alexfh added a comment.

In D103009#2956177 <https://reviews.llvm.org/D103009#2956177>, @xbolva00 wrote:

> Well, I am not sure, your code compiled with GCC will have same issue.

I'm not sure "gcc also has this problem" is a good excuse ;)

> Overallocating is also questionable - your code relies on some many things to work luckily together - not a very ideal state.

Well, it's not my code, it's mysql, which is probably Oracle's code, I suppose ;) But jokes aside, the function I provided as a test case looks totally reasonable to me and it shouldn't zero-fill the allocated memory, if it wasn't asked to. It's good there is a workaround (`-fno-builtin-calloc`) and it seems like some time ago mysql started using this flag in its gcc builds, because this specific optimization caused substantial performance regressions: http://smalldatum.blogspot.com/2017/11/a-new-optimization-in-gcc-5x-and-mysql.html

I guess, mysql is not the only software using its wrappers around malloc + memset with a similar logic.

> This transformation was in simplifylibcalls for very long time anyway

It wasn't **this** transformation. It was targeting specifically `memset(malloc(size), 0, size)` pattern, which is much more specific and safe to replace to `calloc`.

> so if anything, then postdominating check….

Yes, please. More strictly, you could try to check that the memset is on all paths from malloc to where the result of malloc can be used (in any way except for checking it for zero maybe). Not sure whether it can be efficiently implemented though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009