This is an archive of the discontinued LLVM Phabricator instance.

[AtomicExpand] Refactor load/cmpxchg callbacks to make LLSC explicit. NFCI.
ClosedPublic

Authored by ab on Sep 2 2015, 9:07 AM.

Details

Summary

We used to have this magic "hasLLSC()" callback, which really meant two things:

  • expand cmpxchg (to ll/sc).
  • expand atomic loads using ll/sc (rather than cmpxchg).

Instead, introduce explicit callbacks:

  • bool shouldExpandAtomicCmpXchgInIR(inst)
  • AtomicExpansionKind shouldExpandAtomicLoadInIR(inst)

and rename AtomicRMWExpansionKind into AtomicExpansionKind (done in a separate patch, for readability).

In addition to being IMO clearer, this gives TLI more flexibility when deciding which loads/cmpxchgs to expand, and how.

If folks think this is a positive change, I'll update the other backends before committing. Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

ab updated this revision to Diff 33814.Sep 2 2015, 9:07 AM
ab retitled this revision from to [AtomicExpand] Refactor load/cmpxchg callbacks to make LLSC explicit. NFCI..
ab updated this object.
ab added reviewers: morisset, jfb, t.p.northover.
ab added a subscriber: llvm-commits.
ab added a comment.Sep 10 2015, 12:15 PM

Ping!

I also realized this will require a change to the atomics page; I'll do that if people agree!

morisset accepted this revision.Sep 11 2015, 2:50 AM
morisset edited edge metadata.

LGTM, there is just an assert to fix.

lib/CodeGen/AtomicExpandPass.cpp
184 ↗(On Diff #33814)

Typo: s/tryExpandAtomicRMW/tryExpandAtomicLoad/

This revision is now accepted and ready to land.Sep 11 2015, 2:50 AM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/CodeGen/AtomicExpandPass.cpp
509

If ResultingLoad is not null but tryExpandAtomicLoad(ResultingLoad) failed, This method returns true in despite of invalidated RMWI.

ab marked an inline comment as done.Sep 12 2015, 11:55 AM

I couldn't reproduce, but I see the issue: r247514 should fix it.

Fixed. Thanks!