Deprecate constant reference API to DoNotOptimize. #1493

ckennelly · 2022-09-27T16:39:36Z

The compiler assume that a constant reference, even though escaped via asm
volatile, is unchanged. The const-ref interface is deprecated to discourage
new uses of it, as subtle compiler optimizations (invariant hoisting, etc.) can
occur.

Within microbenchmarks for Abseil's hashtables, BM_FindMiss_Hot
(https://github.com/google/fleetbench/blob/c0eaa90671d6cc99eb065864e74f0175bee24a5d/fleetbench/swissmap/hot_swissmap_benchmark.cc#L48)
has a const uint32_t key is passed to to the lookup of a hashtable.
With the key marked const, LLVM hoists part of the lookup
calculation outside of the loop.

With the const removed, this hoisting does not occur.

LebedevRI · 2022-09-27T17:13:30Z

I think the description is missing some code examples.

The compiler assume that a constant reference, even though escaped via asm volatile, is unchanged. The const-ref interface is deprecated to discourage new uses of it, as subtle compiler optimizations (invariant hoisting, etc.) can occur. Within microbenchmarks for Abseil's hashtables, BM_FindMiss_Hot (https://github.com/google/fleetbench/blob/c0eaa90671d6cc99eb065864e74f0175bee24a5d/fleetbench/swissmap/hot_swissmap_benchmark.cc#L48) has a `const uint32_t key` is passed to to the lookup of a hashtable. With the `key` marked `const`, LLVM hoists part of the lookup calculation outside of the loop. With the `const` removed, this hoisting does not occur.

ckennelly · 2022-09-28T01:25:35Z

I think the description is missing some code examples.

Done.

dmah42 · 2022-09-28T09:10:22Z

do any of our tests now trigger this deprecation message?

LebedevRI · 2022-09-28T23:29:54Z

I think the description is missing some code examples.

Done.

Thanks, but i would like to see a more self-contained example on godbolt where said hoisting can be observed.

LebedevRI · 2022-10-31T15:03:50Z

I think the description is missing some code examples.

Done.

Thanks, but i would like to see a more self-contained example on godbolt where said hoisting can be observed.

I'm asking because in general const-ness of the pointer does not give any breathing room to the compiler.
So there is something else going on, and i'm not sure if this will be affecting genuine cases too.

dmah42 · 2022-10-31T15:28:27Z

I think the description is missing some code examples.

Done.

Thanks, but i would like to see a more self-contained example on godbolt where said hoisting can be observed.

https://gcc.godbolt.org/z/EG4PdT9G6 is about as minimal as i can get it on short order that shows the effect with the current code.

LebedevRI · 2022-10-31T16:47:31Z

Thanks! I haven't tried running that yet, but what is the observation there?
Is it really just the different performance, or is RandomNonexistent() call only called once? Or set.find(key)?

LebedevRI · 2022-10-31T16:52:34Z

Also, we have a bit of documentation problem. There, DoNotOptimize() is being used
to literally completely obscure the value, make it completely opaque from compiler,
yet all of the comments say that it is designed to stop the compiler from eliding said value,
but i've never seen any docs that say that it should also obscure it's value.

In the const variant, the key variable is taken by value by DoNotOptimize(),
so SROA successfully promotes it from stack to register,
and in non-const variant that obviously does not happen,
so some perf difference is completely expected.

What happens if you do DoNotOptimize(&key) with the const variant, does it fix it?
Perhaps we need a new set of magic functions ObscureValue() instead.

dmah42 · 2022-11-01T10:23:06Z

Thanks! I haven't tried running that yet, but what is the observation there? Is it really just the different performance, or is RandomNonexistent() call only called once? Or set.find(key)?

it's the hoisting of the RandomNonExistent as per the original comment on the PR. it's unclear how much this specifically is a performance issue, but it indicates a broader class of "missed optimizations" due to the implementation of DoNotOptimize.

i should note, we've applied this patch internally already as it had significant impact across our internal benchmarks.

The const-reference API to DoNotOptimize was deprecated with google#1493. Some examples in the user guide are using exactly that deprecated interface. This fixes that by passing non-const lvalues instead. Fixes google#1566

* Update AUTHORS/CONTRIBUTORS * Fix examples with deprecated DoNotOptimize API The const-reference API to DoNotOptimize was deprecated with #1493. Some examples in the user guide are using exactly that deprecated interface. This fixes that by passing non-const lvalues instead. Fixes #1566

--- Manual changes: --- 1) Updated version of google_benchmark to fix windows compile error. New range: https://chromium.googlesource.com/external/github.com/google/benchmark/+log/e8baf26..b177433 2) Remove unused sleep.[cc|h] from BUILD.gn. (Removed here: google/benchmark#1549) 3) Fix deprecation warnings (treated as errors) in allocation_perf.cc. benchmark::DoNotOptimize has deprecated overloads due to google/benchmark#1493. Bug: 1425054 --- Original message generated by autoroll: --- Rolling v8/third_party/google_benchmark/src: https://chromium.googlesource.com/external/github.com/google/benchmark/+log/e8baf26..efc89f0 link to benchmark directly for tests that aren't link_main_test (#1576) (dominic) https://chromium.googlesource.com/external/github.com/google/benchmark/+/efc89f0 Convert uses of `const char*` to `std::string` (#1567) (dominic) https://chromium.googlesource.com/external/github.com/google/benchmark/+/46d3c84 add '@' to correctly reference build file for libpfm (#1575) (dominic) https://chromium.googlesource.com/external/github.com/google/benchmark/+/68aa190 Address warnings on NVIDIA nvc++ (#1573) (Henrique Bucher) https://chromium.googlesource.com/external/github.com/google/benchmark/+/9f7dc38 simplify setting C++ standard (Dominic Hamon) https://chromium.googlesource.com/external/github.com/google/benchmark/+/1b507cb [FR] Provide public accessors to benchmark name and arguments #1551 (#1563) (Mike Apodaca) https://chromium.googlesource.com/external/github.com/google/benchmark/+/f32748c use std::string for skip messages (#1571) (dominic) https://chromium.googlesource.com/external/github.com/google/benchmark/+/060d762 [FR] state.SkipWithMessage #963 (#1564) (Mike Apodaca) https://chromium.googlesource.com/external/github.com/google/benchmark/+/adb0d3d ... Change-Id: I69218b84b88fb1da25e740d7fb623528d526b85b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4429376 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Matthias Liedtke <mliedtke@chromium.org> Cr-Commit-Position: refs/heads/main@{#87139}

…#35459) ### Rationale for this change Google Benchmark 1.6.1 added `benchmark::MemoryManager::Stop(Result&)` and deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark 1.8.0 dropped deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark deprecated `DoNotOptimize(const &)`: google/benchmark#1493 ### What changes are included in this PR? We can always use `benchmark::MemoryManager::Stop(Result&)` by requiring Google Benchmark 1.6.1 or later. Don't use deprecated `DoNotOptimize(const &)`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: #35458 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

… later (apache#35459) ### Rationale for this change Google Benchmark 1.6.1 added `benchmark::MemoryManager::Stop(Result&)` and deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark 1.8.0 dropped deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark deprecated `DoNotOptimize(const &)`: google/benchmark#1493 ### What changes are included in this PR? We can always use `benchmark::MemoryManager::Stop(Result&)` by requiring Google Benchmark 1.6.1 or later. Don't use deprecated `DoNotOptimize(const &)`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#35458 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…#35459) ### Rationale for this change Google Benchmark 1.6.1 added `benchmark::MemoryManager::Stop(Result&)` and deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark 1.8.0 dropped deprecated `benchmark::MemoryManager::Stop(Result*)`. Google Benchmark deprecated `DoNotOptimize(const &)`: google/benchmark#1493 ### What changes are included in this PR? We can always use `benchmark::MemoryManager::Stop(Result&)` by requiring Google Benchmark 1.6.1 or later. Don't use deprecated `DoNotOptimize(const &)`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: #35458 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

bgaifullin · 2023-06-12T15:46:27Z

This patch breaks using DoNotOptimize with rvalue (see #1584)

… operands (#79924) Modify #77393 to clear shadow memory using `llvm.memset.*` when the size is large, similar to `shouldUseBZeroPlusStoresToInitialize` in clang for `-ftrivial-auto-var-init=`. The intrinsic, if lowered to libcall, will use the msan interceptor. The instruction selector lowers a `StoreInst` to multiple stores, not utilizing `memset`. When the size is large (e.g. `store { [100 x i32] } zeroinitializer, ptr %12, align 1`), the generated code will be long (and `CodeGenPrepare::optimizeInst` will even crash for a huge size). ``` // Test stack size template <class T> void DoNotOptimize(const T& var) { // deprecated by google/benchmark#1493 asm volatile("" : "+m"(const_cast<T&>(var))); } int main() { using LargeArray = std::array<int, 1000000>; auto large_stack = []() { DoNotOptimize(LargeArray()); }; /////// CodeGenPrepare::optimizeInst triggers an assertion failure when creating an integer type with a bit width>2**23 large_stack(); } ```

ckennelly force-pushed the master branch from 1246945 to b1b8509 Compare September 27, 2022 16:51

ckennelly force-pushed the master branch from b1b8509 to 55ca62a Compare September 28, 2022 01:25

Merge branch 'main' into master

0f360ea

Merge branch 'main' into master

cd9c582

dmah42 approved these changes Feb 6, 2023

View reviewed changes

dmah42 merged commit 53df805 into google:main Feb 6, 2023

mjacobse mentioned this pull request Mar 7, 2023

[BUG] Examples from User Guide trigger deprecation warning for const-reference DoNotOptimize #1566

Closed

mjacobse mentioned this pull request Mar 7, 2023

Fix examples in user guide using deprecated DoNotOptimize-API #1568

Merged

davidstone mentioned this pull request Apr 9, 2023

[BUG] DoNotOptimize(T &&) should not be deprecated #1584

Closed

kou mentioned this pull request May 6, 2023

GH-35458: [C++][Benchmarking] Require Google Benchmark 1.6.1 or later apache/arrow#35459

Merged

LebedevRI mentioned this pull request Jul 4, 2023

[BUG] DoNotOptimize const & overload chosen for non-const lvalue references #1619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate constant reference API to DoNotOptimize. #1493

Deprecate constant reference API to DoNotOptimize. #1493

ckennelly commented Sep 27, 2022 •

edited by LebedevRI

Loading

LebedevRI commented Sep 27, 2022 •

edited

Loading

ckennelly commented Sep 28, 2022

dmah42 commented Sep 28, 2022

LebedevRI commented Sep 28, 2022

LebedevRI commented Oct 31, 2022

dmah42 commented Oct 31, 2022

LebedevRI commented Oct 31, 2022

LebedevRI commented Oct 31, 2022 •

edited

Loading

dmah42 commented Nov 1, 2022

bgaifullin commented Jun 12, 2023 •

edited

Loading

Deprecate constant reference API to DoNotOptimize. #1493

Deprecate constant reference API to DoNotOptimize. #1493

Conversation

ckennelly commented Sep 27, 2022 • edited by LebedevRI Loading

LebedevRI commented Sep 27, 2022 • edited Loading

ckennelly commented Sep 28, 2022

dmah42 commented Sep 28, 2022

LebedevRI commented Sep 28, 2022

LebedevRI commented Oct 31, 2022

dmah42 commented Oct 31, 2022

LebedevRI commented Oct 31, 2022

LebedevRI commented Oct 31, 2022 • edited Loading

dmah42 commented Nov 1, 2022

bgaifullin commented Jun 12, 2023 • edited Loading

ckennelly commented Sep 27, 2022 •

edited by LebedevRI

Loading

LebedevRI commented Sep 27, 2022 •

edited

Loading

LebedevRI commented Oct 31, 2022 •

edited

Loading

bgaifullin commented Jun 12, 2023 •

edited

Loading