Page MenuHomePhabricator

Remove dead zero store to calloc initialized memory
ClosedPublic

Authored by reames on May 28 2014, 2:47 PM.

Details

Summary

Optimize the following IR:

%1 = tail call noalias i8* @calloc(i64 1, i64 4)
%2 = bitcast i8* %1 to i32*
; This store is dead and should be removed
store i32 0, i32* %2, align 4

Memory returned by calloc is guaranteed to be zero initialized. If the value being stored is the constant zero (and the store is not otherwise observable across threads), we can delete the store.

Note: I do not have commit rights and will need someone to commit this change after review.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 9889.May 28 2014, 2:47 PM
reames retitled this revision from to Remove dead zero store to calloc initialized memory.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: dexonsmith, nlewycky.
reames added a subscriber: Unknown Object (MLST).

In general I understand the intent of the optimization, but is this really that common of a case to warrant optimization? Was this inspired by real world code patterns?

lib/Transforms/Scalar/DeadStoreElimination.cpp
550 ↗(On Diff #9889)

A few minor formatting nits:

  1. Space between 'if' and '('.
  2. No spaces around the expression.
  3. Looks like the full expression can fit on one line.

Also, in practice I am not sure if this is a problem, but in theory shouldn't this only apply to integer zero? The C Standard notes that the zero representation for floating point and null pointers can be different than all bits zero. Again, not sure if that is actually an issue, but wanted to raise the point anyway.

572 ↗(On Diff #9889)

Not sure if it is worth it to merge, but this code is very similar to the block before it.

In D3942#4, @meadori wrote:

In general I understand the intent of the optimization, but is this really that common of a case to warrant optimization? Was this inspired by real world code patterns?

For C, not really. For other languages yes. For example, most Java implementations zero initialize all memory (since the default value of every field is defined to be zero), but is common for programmers to write manual initializers anyways. While the allocation function isn't actually calloc, the semantics are quite similar. In practice, we use a "is-calloc-like" attribute on the allocation function. I may propose this for upstreaming at some point, but wanted to get supporting optimizations in first and calloc provides a good stand in.

lib/Transforms/Scalar/DeadStoreElimination.cpp
550 ↗(On Diff #9889)

Will fix the formatting.

The constant zero point is a good one. However, LLVM widely appears to mix null and zero. I'm furthering an existing problem, not creating a new one. Also, this is purely a theoretical issue at the moment. No system I'm aware of as NULL as anything other than the zero address.

572 ↗(On Diff #9889)

The code is in fact nearly identical, but the handling is slightly different. I would resist the merge for the moment, but if you really want it, I will do so.

reames updated this revision to Diff 10572.Jun 18 2014, 10:12 AM

Updated to reflect review comments. Sorry for the delay, this fell off my mental todo list.

nicholas accepted this revision.Jun 29 2014, 1:30 PM
nicholas added a reviewer: nicholas.
nicholas added a subscriber: nicholas.

LGTM

lib/Transforms/Scalar/DeadStoreElimination.cpp
537 ↗(On Diff #10572)

"Value* V" -> "Value *V"

This revision is now accepted and ready to land.Jun 29 2014, 1:30 PM
reames closed this revision.Aug 5 2014, 10:57 AM
reames updated this revision to Diff 12203.

Closed by commit rL214897 (authored by @reames).