Page MenuHomePhabricator

[SimplifyCFG] Improve store speculation check
ClosedPublic

Authored by nikic on Jul 24 2021, 1:47 AM.

Details

Summary

isSafeToSpeculateStore() looks for a preceding store to the same location to make sure that introducing a new store of the same value is safe. It currently bails on intervening mayHaveSideEffect() instructions. However, I believe just checking mayWriteToMemory() is sufficient there -- we just need to make sure that we know which value was stored, we don't care if we can unwind in the meantime.

While looking into this, I started having some doubts about the correctness of the transform with regard to thread safety. While we don't try to hoist non-simple stores, I believe we also need to make sure that the preceding store is simple as well. Otherwise we could introduce a spurious non-atomic write after an atomic write -- I believe under our memory model this would result in a subsequent undef atomic read, even if the second write stores the same value as the first.

Example: https://alive2.llvm.org/ce/z/q_3YAL

Diff Detail

Event Timeline

nikic created this revision.Jul 24 2021, 1:47 AM
nikic requested review of this revision.Jul 24 2021, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2021, 1:47 AM

looks reasonable to me.

lebedev.ri accepted this revision.Jul 25 2021, 6:27 AM
lebedev.ri edited the summary of this revision. (Show Details)

I think this should be fine.
The check does seem sufficient, at least for readonly/readnone calls: https://alive2.llvm.org/ce/z/q_3YAL

This revision is now accepted and ready to land.Jul 25 2021, 6:28 AM
This revision was landed with ongoing or failed builds.Jul 26 2021, 6:03 AM
This revision was automatically updated to reflect the committed changes.