All uses of #pragma optimize should be audited |
||
Issue descriptionMany uses of #pragma optimize in Chrome are there to work around VS compiler bugs - they therefore make no sense when we are using clang-cl as our compiler. Others are there due to misguided understandings of how optimizations work. Clearly none of them are strictly necessary because clang-cl actually just *ignores* #pragma optimize - see https://bugs.llvm.org/show_bug.cgi?id=39146. We should go through and clean up some of these, either removing them or adding appropriate #ifdefs so that clang-cl will ignore them. Or, if they are needed for clang-cl we should add the appropriate #ifdefs so that #pragma clang optimize off is used instead. In particular, MSVC_DISABLE_OPTIMIZE should work and it currently doesn't. The clang-cl team *may* implement #pragma optimize and that will affect our strategy for this bug.
,
Oct 1
I did audit this a while ago, see https://bugs.chromium.org/p/chromium/issues/detail?id=505314#c13
,
Oct 1
That's good. I think we should go to the next step and remove them from Chromium's code and either update the compiler to support #pragma optimize or update MSVC_DISABLE_OPTIMIZE to not use it. Right now the pragmas are a bit of a lie to readers.
,
Oct 1
Yup, that sounds good to me :-) (cf https://bugs.chromium.org/p/chromium/issues/detail?id=505314#c24) Just didn't get around to it yet.
,
Dec 19
MSVC_DISABLE_OPTIMIZE is no longer used so I am cleaning it up and renaming it to be less dangerous.
However! #pragma optimize("", off ) is used in quite a few places. It shows up in midl generated code (!!!) and in a few random places in our code. There are also four places in the VS/SDK files where #pragma optimize is used to change optimization settings.
Since this pragma is ignored by clang these places are all clearly unnecessary and we can delete all of the ones in our own code, but the ones in the VS/SDK files and the midl generated files will make it harder to remove -Wno-ignored-pragma-optimize.
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/635414565ab43f1234b6df42df72d115cde7a63b commit 635414565ab43f1234b6df42df72d115cde7a63b Author: Bruce Dawson <brucedawson@chromium.org> Date: Thu Dec 20 03:09:45 2018 Remove several uses of #pragma optimize Because clang-cl ignores #pragma optimize that means that we have been running an experiment to prove the lack of need for #pragma optimize for the last year. Removing the provably unneeded directives helps to set a good example and avoid confusion. The one *possible* case where this CL could cause problems is in base::debug::Alias where clang was disabling optimizations on non-Windows platforms. I still think that is incorrect and that what is needed is to ensure that alias.cc is excluded from LTO on whatever platforms support it, but for minimum risk we could restore the behavior on non-Windows platforms. This will change the behavior for users of base who still compile with VC++ (those outside of Chromium) but I am confident that this is fine. Bug: 891075 Change-Id: Ie63e6cc1c3eb9ab6f81855afc04b06e414a3b2af Reviewed-on: https://chromium-review.googlesource.com/c/1384986 Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#618092} [modify] https://crrev.com/635414565ab43f1234b6df42df72d115cde7a63b/base/debug/alias.cc [modify] https://crrev.com/635414565ab43f1234b6df42df72d115cde7a63b/base/debug/stack_trace_win.cc [modify] https://crrev.com/635414565ab43f1234b6df42df72d115cde7a63b/base/test/test_suite.cc [modify] https://crrev.com/635414565ab43f1234b6df42df72d115cde7a63b/base/win/process_startup_helper.cc
,
Jan 10
Microsoft has been told about the midl generated code that uses #pragma optimize and the others look harmless (mostly conditional for VC++ bugs). Closing.
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f141f576516b43cce6fa34e29530be7df3b9a40d commit f141f576516b43cce6fa34e29530be7df3b9a40d Author: Bruce Dawson <brucedawson@chromium.org> Date: Thu Jan 10 19:06:36 2019 Update and rename MSVC_DISABLE_OPTIMIZE for clang It is sometimes handy to disable optimizations for a particular source file for easier debugging. MSVC_DISABLE_OPTIMIZE had beeen (mis)used for this purpose except that clang-cl currently ignores the VC++ #pragma optimize flag. This change renames the macro to DISABLE_OPTIMIZE, makes it work for clang and VC++, and prevents it from being used in official builds. This bug tracks the request to get clang-cl to support the VC++ syntax, but that comes with tradeoffs: https://bugs.llvm.org/show_bug.cgi?id=39146 Bug: 891075 Change-Id: I1bf7aa60cb44f4834ab6d30da43ed918258879fd Reviewed-on: https://chromium-review.googlesource.com/c/1284270 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#621681} [modify] https://crrev.com/f141f576516b43cce6fa34e29530be7df3b9a40d/base/compiler_specific.h |
||
►
Sign in to add a comment |
||
Comment 1 by brucedaw...@chromium.org
, Oct 1