Page MenuHomePhabricator

[SDAG] Update generic code to conservatively check for isAtomic in addition to isVolatile
ClosedPublic

Authored by reames on Aug 15 2019, 4:07 PM.

Details

Summary

See D66309 for context.

This is the first sweep of generic code to add isAtomic bailouts where appropriate. The intention here is to have the switch from AtomicSDNode to LoadSDNode/StoreSDNode be close to NFC; that is, I'm not looking to allow additional optimizations at this time. That will come later.

(Note: This is currently based on ToT without D66309. That's why there are no test diffs with the experimental flag visible. It could be added either before or after that change.)

Diff Detail

Event Timeline

reames created this revision.Aug 15 2019, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 4:07 PM
reames updated this revision to Diff 215493.Aug 15 2019, 4:13 PM

update all todos with an easily grepable tag for ease of future work

reames planned changes to this revision.Mon, Sep 9, 1:29 PM

Waiting behind D66322 since that's where discussion has happened. Once that's in, will rebase with the same changes here.

Porting a comment from craig.topper over on D66322:
"Will you also be updating nonvolatile_load/nonvolatile_store in include/llvm/Target/TargetSelectionDAG.td. We use those in several places where we change the size of a load."

Needs addressed in rebase.

reames updated this revision to Diff 219583.Tue, Sep 10, 12:22 PM

Rebase to include isSimple usage, and test updates.

Still need to include the nonvolatile_load/nonvolatile_store cases. Need access to a larger build machine for that one. :)

reames updated this revision to Diff 219585.Tue, Sep 10, 12:35 PM

Add the nonvolatile_load/store cases. I'll rename them in a separate commit so as not to clutter the diff on this one.

With that, we're catching *most* of the atomic vs non-atomic differences. Skimming the remaining test diffs, I clearly need to adjust my test lines to hide O3 vs experimental O3 differences. There do appear to be a few remaining cases that are being miscompiled, mostly around widenable store idioms. Need to track that down in a future patch.

arsenm accepted this revision.Thu, Sep 12, 3:12 PM
arsenm added a subscriber: arsenm.

LGTM with nits

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14036 ↗(On Diff #219585)

Capitalize comment

16320 ↗(On Diff #219585)

Ditto

16332 ↗(On Diff #219585)

Ditto

16464 ↗(On Diff #219585)

Ditto

This revision is now accepted and ready to land.Thu, Sep 12, 3:12 PM
This revision was automatically updated to reflect the committed changes.