This is an archive of the discontinued LLVM Phabricator instance.

[NFC][sanitizer] Atomix relaxed in TwoLevelMap
ClosedPublic

Authored by vitalybuka on Oct 24 2021, 7:32 PM.

Details

Summary

This is NOOP in x86_64.
On arch64 it avoids Data Memory Barrier with visible improvements on micro benchmarks.

Diff Detail

Event Timeline

vitalybuka requested review of this revision.Oct 24 2021, 7:32 PM
vitalybuka created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2021, 7:32 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka retitled this revision from [NFC][sanitizer] Atomix relaxed in TwoLevelMap This is NOOP in x86_64. On arch64 it avoid Data Memory Barrier and visible on micro benchmarks. to [NFC][sanitizer] Atomix relaxed in TwoLevelMapThis is NOOP in x86_64.On arch64 it avoid Data Memory Barrier and visibleon micro benchmarks..Oct 24 2021, 7:35 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka retitled this revision from [NFC][sanitizer] Atomix relaxed in TwoLevelMapThis is NOOP in x86_64.On arch64 it avoid Data Memory Barrier and visibleon micro benchmarks. to [NFC][sanitizer] Atomix relaxed in TwoLevelMap.
vitalybuka edited the summary of this revision. (Show Details)
dvyukov added inline comments.Oct 24 2021, 11:13 PM
compiler-rt/lib/sanitizer_common/sanitizer_flat_map.h
128

The new code is incorrect. So I would add an explanatory comment that this is intentional and why we assume this will work in practice. Otherwise the next maintainer may revert this.

149

I assume this part does not affect performance, right? And the store side has much higher chances of causing real problems. So I would keep the release here.

vitalybuka marked an inline comment as done.

fixes

vitalybuka marked an inline comment as done.Oct 25 2021, 12:35 AM
vitalybuka edited the summary of this revision. (Show Details)
dvyukov accepted this revision.Oct 25 2021, 10:10 PM

LGTM with a nit

compiler-rt/lib/sanitizer_common/sanitizer_flat_map.h
133

The problem is with the other case: the load returns non-nullptr, but we don't have proper visibility over the returned memory.
I think we also need to say that this code is incorrect and the reason for keeping incorrect code is performance.
Something along the following lines:

This code needs to use memory_order_acquire/consume, but we use memory_order_relaxed for performance reasons (matters for arm64). We expect memory_order_relaxed to be effectively equivalent to memory_order_consume in this case for all relevant architectures: all dependent data is reachable only by dereferencing the resulting pointer.

This revision is now accepted and ready to land.Oct 25 2021, 10:10 PM
This revision was automatically updated to reflect the committed changes.