This is an archive of the discontinued LLVM Phabricator instance.

Allow ImproveChain to get past relaxed atomics
Needs ReviewPublic

Authored by yoni.lavi on Mar 10 2022, 2:51 AM.

Details

Summary

As titled; Relaxed AKA Monotonic atomics don't impose any "global" synchronization semantics between threads.
Any other non-aliasing mem operation can treat them the same as an ordinary load (or load/modify/store) so it should not preclude chain improvement.

The case of aliasing load being reordered around an atomic load of the same deserves mention here:

  • Technically, reordering changes program semantics, in the sense that:
    • loading x after atomically loading x, the non-atomic load cannot give us a result that is more stale than the atomic one
    • loading x before atomically loading x, the non-atomic load can result in an arbitrarily stale value
  • We still allow this, on the grounds that:
    • If nothing is concurrently modifying x, there is no difference.
    • If any concurrent writers to x potentially exist, both versions of the program suffer from UB due to a data race, so it is legit to alter semantics in this manner.

Diff Detail

Event Timeline

yoni-lavi created this revision.Mar 10 2022, 2:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 2:51 AM
yoni-lavi requested review of this revision.Mar 10 2022, 2:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 2:51 AM
yoni-lavi edited the summary of this revision. (Show Details)Mar 10 2022, 2:59 AM
yoni-lavi added reviewers: niravd, damageboy, ibookstein.
yoni-lavi changed 2 blocking reviewer(s), added 1: ibookstein; removed 1: niravd.Mar 10 2022, 3:01 AM
yoni-lavi updated this revision to Diff 414329.Mar 10 2022, 3:17 AM
yoni-lavi edited the summary of this revision. (Show Details)
  • lint fixes
yoni-lavi updated this revision to Diff 414343.Mar 10 2022, 4:33 AM
  • Fixed getOrdering() -> getMergedOrdering()
yoni-lavi updated this revision to Diff 414620.Mar 11 2022, 2:11 AM
  • Emptying my diff just to check test stability
yoni-lavi updated this revision to Diff 414624.Mar 11 2022, 2:40 AM
  • Restore original

I've confirmed my diff does not cause any test failures that aren't already present in the base revision (latest main).

It does cause ThreadSanitizer-x86_64 :: restore_stack.cpp to time out, Though test may well be flaky in that regard; it also doesn't contain any relaxed atomics as far as I can tell.

  • rb to re-test
yoni-lavi updated this revision to Diff 417543.Mar 23 2022, 2:51 AM
  • removing all content to check for flaky test timeouts again
yoni-lavi updated this revision to Diff 417565.Mar 23 2022, 4:26 AM

restored content, without debug prints

RKSimon added a subscriber: RKSimon.

I've confirmed my diff does not cause any test failures that aren't already present in the base revision (latest main).

Do you have tests that you can add that do show a change?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
24048

is this how clang-format formats the code?

craig.topper added inline comments.Mar 23 2022, 11:43 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
24043

I'm not sure what this comment is trying to say

I can definitely come up with a test case where the SDAG is different.

However, strictly speaking, weakening the chain dependencies doesn't ever "force" a change in the output instruction schedule.

So a good test would have to assert on the SDAG structure rather than the output schedule / machine instructions.

Is there an existing framework to assert on the SDAG for a given basic block in an example program?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
24043

It's related to the case above (LSBaseSDNode) where Offset is computed to account for PRE_INC / PRE_DEC.

If I understood it right, this way of representing ptr increments as part of the memory access itself does not exist for the "atomic" SDNodes. Hence the Offset = 0 and this comment.

24048

yes. (the phabricator build if clang-format wants to re-format your code...)

yoni-lavi added inline comments.Mar 24 2022, 6:04 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
24048

no idea why editing isn't allowed ... but, I meant to write: "the phabricator build fails if ..."

craig.topper added inline comments.Mar 24 2022, 8:57 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
24043

The use "allegedly" and "issue" make it seem like there might be some like might be some problem or question here. Can you write it in a more confident way. Like "Atomic instructions don't have an offset"

yoni.lavi commandeered this revision.Dec 23 2022, 11:51 AM
yoni.lavi added a reviewer: yoni-lavi.
yoni.lavi added a subscriber: yoni.lavi.

Created new account here, bound to my github account.
I will also rebase this and attempt to add tests.