This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Value forwarding for unordered atomics
ClosedPublic

Authored by reames on Dec 8 2015, 10:05 AM.

Details

Summary

This patch teaches the fully redundant load part of EarlyCSE how to forward from atomic and volatile loads and stores, and how to eliminate unordered atomics (only). This patch does not include dead store elimination support for unordered atomics, that will follow in the near future.

The basic idea is that we allow all loads and stores to be tracked by the AvailableLoad table. We store a bit in the table which tracks whether load/store was atomic, and then only replace atomic loads with ones which were also atomic.

No attempt is made to refine our handling of ordered loads or stores. Those are still treated as full fences. We could pretty easily extend the release fence handling to release stores, but that should be a separate patch

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 42188.Dec 8 2015, 10:05 AM
reames retitled this revision from to [EarlyCSE] Value forwarding for unordered atomics.
reames updated this object.
reames added a subscriber: llvm-commits.
dberlin edited edge metadata.Dec 8 2015, 10:17 AM

At a glance, this looks correct to me, and to be honest, Phillip almost certainly understands atomics better than I do ;)

lib/Transforms/Scalar/EarlyCSE.cpp
624 ↗(On Diff #42188)

At least at a glance, this looks like you compare a bool with a bool using >=, which is pretty confusing?

This LGTM but another pair of eyes wouldn't hurt.

lib/Transforms/Scalar/EarlyCSE.cpp
285 ↗(On Diff #42188)

Please end this with a period.

303 ↗(On Diff #42188)

You do not initialize IsAtomic in the default constructor. It might be nicer to use the NSDMI-style to make it stand out more.

610 ↗(On Diff #42188)

Please end this sentence with a period.

621 ↗(On Diff #42188)

Please end this sentence with a period.

727 ↗(On Diff #42188)

Please end this with a period.

test/Transforms/EarlyCSE/atomics.ll
17–22 ↗(On Diff #42188)

Instead of relying on the calls to SimplifyInstruction to wipe away the sub, why not just ret %b and check for ret i32 %a ?

reames added inline comments.Dec 8 2015, 11:50 AM
lib/Transforms/Scalar/EarlyCSE.cpp
303 ↗(On Diff #42188)

Ouch! Good catch. Thanks.

624 ↗(On Diff #42188)

That is what I'm doing. This really calls for an implication operator, but C++ doesn't have one of those? I could extract out a helper function I guess to give it a name.

test/Transforms/EarlyCSE/atomics.ll
17–22 ↗(On Diff #42188)

Because then %a becomes dead and there's nothing preventing us from removing it instead of forwarding.

This revision was automatically updated to reflect the committed changes.
jfb edited edge metadata.Dec 9 2015, 2:26 AM

Sorry it took a while to get to this, I'm in a different timezone. Looks good, I added some tests in D15371 to convince myself that things were working as I expect.