Skip to content

[clang-cl] Map /Ot to -O3 instead of -O2 #95406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

zmodem
Copy link
Collaborator

@zmodem zmodem commented Jun 13, 2024

/Ot (which is also implied by /O2) is supposed to optimize for maximum speed, so -O3 seems like a better match.

/Ot (which is also implied by /O2) is supposed to optimize for maximum
speed, so -O3 seems like a better match.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jun 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 13, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Hans (zmodem)

Changes

/Ot (which is also implied by /O2) is supposed to optimize for maximum speed, so -O3 seems like a better match.


Full diff: https://github.com/llvm/llvm-project/pull/95406.diff

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4)
  • (modified) clang/docs/UsersManual.rst (+2-2)
  • (modified) clang/include/clang/Driver/Options.td (+2-2)
  • (modified) clang/lib/Driver/ToolChains/MSVC.cpp (+2-2)
  • (modified) clang/test/Driver/cl-options.c (+5-5)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8c2f737836a9d..68355dbb5861b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -921,6 +921,10 @@ Android Support
 Windows Support
 ^^^^^^^^^^^^^^^
 
+- The clang-cl ``/Ot`` compiler option ("optimize for speed", also implied by
+  ``/O2``) now maps to clang's ``-O3`` optimizataztion level instead of ``-O2``.
+  Users who prefer the old behavior can use ``clang-cl /Ot /clang:-O2 ...``.
+
 - Clang-cl now supports function targets with intrinsic headers. This allows
   for runtime feature detection of intrinsics. Previously under clang-cl
   ``immintrin.h`` and similar intrinsic headers would only include the intrinsics
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index f954857b0235a..ee30e4eff9ea0 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -4636,8 +4636,8 @@ Execute ``clang-cl /?`` to see a list of supported options:
       /Og                     No effect
       /Oi-                    Disable use of builtin functions
       /Oi                     Enable use of builtin functions
-      /Os                     Optimize for size
-      /Ot                     Optimize for speed
+      /Os                     Optimize for size (like clang -Os)
+      /Ot                     Optimize for speed (like clang -O3)
       /Ox                     Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead
       /Oy-                    Disable frame pointer omission (x86 only, default)
       /Oy                     Enable frame pointer omission (x86 only)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 880221176027e..1620b12d55ed9 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8279,9 +8279,9 @@ def : CLFlag<"Oi">, Alias<_SLASH_O>, AliasArgs<["i"]>,
 def : CLFlag<"Oi-">, Alias<_SLASH_O>, AliasArgs<["i-"]>,
   HelpText<"Disable use of builtin functions">;
 def : CLFlag<"Os">, Alias<_SLASH_O>, AliasArgs<["s"]>,
-  HelpText<"Optimize for size">;
+  HelpText<"Optimize for size (like clang -Os)">;
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
-  HelpText<"Optimize for speed">;
+  HelpText<"Optimize for speed (like clang -O3)">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
   HelpText<"Deprecated (like /Og /Oi /Ot /Oy /Ob2); use /O2">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index b7021d4b996dd..d03687208c5c6 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -861,7 +861,7 @@ static void TranslateOptArg(Arg *A, llvm::opt::DerivedArgList &DAL,
           DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "s");
         } else if (OptChar == '2' || OptChar == 'x') {
           DAL.AddFlagArg(A, Opts.getOption(options::OPT_fbuiltin));
-          DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "2");
+          DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "3");
         }
         if (SupportsForcingFramePointer &&
             !DAL.hasArgNoClaim(options::OPT_fno_omit_frame_pointer))
@@ -901,7 +901,7 @@ static void TranslateOptArg(Arg *A, llvm::opt::DerivedArgList &DAL,
       DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "s");
       break;
     case 't':
-      DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "2");
+      DAL.AddJoinedArg(A, Opts.getOption(options::OPT_O), "3");
       break;
     case 'y': {
       bool OmitFramePointer = true;
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 2c17459dde656..e77ec364170d1 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -189,12 +189,12 @@
 // RUN: %clang_cl /Ot --target=i686-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ot %s
 // RUN: %clang_cl /Ot --target=x86_64-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ot %s
 // Ot: -mframe-pointer=none
-// Ot: -O2
+// Ot: -O3
 
 // RUN: %clang_cl /Ox --target=i686-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ox %s
 // RUN: %clang_cl /Ox --target=x86_64-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ox %s
 // Ox: -mframe-pointer=none
-// Ox: -O2
+// Ox: -O3
 
 // RUN: %clang_cl --target=i686-pc-win32 /O2sy- -### -- %s 2>&1 | FileCheck -check-prefix=PR24003 %s
 // PR24003: -mframe-pointer=all
@@ -202,14 +202,14 @@
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror -Wno-msvc-not-found /Oy- /O2 -### -- %s 2>&1 | FileCheck -check-prefix=Oy_2 %s
 // Oy_2: -mframe-pointer=all
-// Oy_2: -O2
+// Oy_2: -O3
 
 // RUN: %clang_cl --target=aarch64-pc-windows-msvc -Werror -Wno-msvc-not-found /Oy- /O2 -### -- %s 2>&1 | FileCheck -check-prefix=Oy_aarch64 %s
 // Oy_aarch64: -mframe-pointer=non-leaf
-// Oy_aarch64: -O2
+// Oy_aarch64: -O3
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror -Wno-msvc-not-found /O2 /O2 -### -- %s 2>&1 | FileCheck -check-prefix=O2O2 %s
-// O2O2: "-O2"
+// O2O2: "-O3"
 
 // RUN: %clang_cl /Zs -Werror /Oy -- %s 2>&1
 

@zmodem zmodem added the clang-cl `clang-cl` driver. Don't use for other compiler parts label Jun 13, 2024
@zmodem
Copy link
Collaborator Author

zmodem commented Jun 13, 2024

@AaronBallman as discussed on https://discourse.llvm.org/t/clang-cl-optimization-option/79554

Also @nico who IIRC looked at these flags in recent times.

@nico
Copy link
Contributor

nico commented Jun 13, 2024

Can you add something like "use /O2 /clang:-O2 to restore the previous behavior" to the commit message, on case someone prefers that?

Otherwise LG.

@nico
Copy link
Contributor

nico commented Jun 13, 2024

Oh, you have that in the release notes already, even better.

@zmodem zmodem merged commit da249ca into llvm:main Jun 14, 2024
12 checks passed
@efriedma-quic
Copy link
Collaborator

If we're trying to match MSVC, the amount of inlining MSVC does at /O2 is probably closer to what clang does at -O2 than -O3. Which is why it was mapped that way in 015ce0f, I think. clang's -O3 is really aggressive (which tends to look good in benchmarks, but less so in real software).

I'm a bit worried we're going to get a wave of complaints about codesize in reaction to this.

@efriedma-quic
Copy link
Collaborator

For reference, current MSVC has a flag /Ob3 to request more aggressive inlining.

jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.libchrome that referenced this pull request Jun 22, 2024
…14561-gecea8371-1 / 32dd3795bce8b347fda786529cf5e42a813e0b7d-2 : 3cf924b934322fd7b514600a7dc84fc517515346-1

https://chromium.googlesource.com/external/github.com/llvm/llvm-project/+log/084e2b53..ecea8371

https://chromium.googlesource.com/external/github.com/rust-lang/rust/+log/32dd3795bce8..3cf924b93432

Also update clang-cl flags to pass "/clang:-O2" after /O2
after  llvm/llvm-project#95406

Ran:
./tools/clang/scripts/upload_revision.py ecea8371ff03c15fb3dc27ee4108b98335fd2d63
tools/clang/scripts/sync_deps.py
tools/rust/gnrt_stdlib.py
third_party/abseil-cpp/generate_def_files.py

Bug: 340924169
Change-Id: Ie3680f33261952f66d972398656661090c47726a
Disable-Rts: True
Binary-Size: Inlining heuristic change, see crbug.com/347984812.
Cq-Include-Trybots: chromium/try:chromeos-amd64-generic-cfi-thin-lto-rel
Cq-Include-Trybots: chromium/try:dawn-win10-x86-deps-rel
Cq-Include-Trybots: chromium/try:lacros-arm64-generic-rel
Cq-Include-Trybots: chromium/try:linux-chromeos-dbg
Cq-Include-Trybots: chromium/try:linux_chromium_cfi_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_chromeos_msan_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_msan_rel_ng
Cq-Include-Trybots: chromium/try:mac11-arm64-rel,mac_chromium_asan_rel_ng
Cq-Include-Trybots: chromium/try:ios-catalyst,win-asan,android-official
Cq-Include-Trybots: chromium/try:fuchsia-arm64-cast-receiver-rel
Cq-Include-Trybots: chromium/try:mac-official,linux-official
Cq-Include-Trybots: chromium/try:win-official,win32-official
Cq-Include-Trybots: chromium/try:win-arm64-rel
Cq-Include-Trybots: chromium/try:linux-swangle-try-x64,win-swangle-try-x86
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-rel
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-rel
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-rel
Cq-Include-Trybots: chrome/try:iphone-device,ipad-device
Cq-Include-Trybots: chrome/try:linux-chromeos-chrome
Cq-Include-Trybots: chrome/try:win-chrome,win64-chrome,linux-chrome,mac-chrome
Cq-Include-Trybots: chrome/try:linux-pgo,mac-pgo,win32-pgo,win64-pgo
Cq-Include-Trybots: chromium/try:android-rust-arm32-rel
Cq-Include-Trybots: chromium/try:android-rust-arm64-dbg
Cq-Include-Trybots: chromium/try:android-rust-arm64-rel
Cq-Include-Trybots: chromium/try:linux-rust-x64-dbg
Cq-Include-Trybots: chromium/try:linux-rust-x64-rel
Cq-Include-Trybots: chromium/try:mac-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5633975
Reviewed-by: Nico Weber <[email protected]>
Commit-Queue: Hans Wennborg <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317359}


CrOS-Libchrome-Original-Commit: 65d491f36bac46186a87e0cc890bb810dc66c4f1
kenhuidotme pushed a commit to kenhuidotme/chromium-build that referenced this pull request Jun 25, 2024
…14561-gecea8371-1 / 32dd3795bce8b347fda786529cf5e42a813e0b7d-2 : 3cf924b934322fd7b514600a7dc84fc517515346-1

https://chromium.googlesource.com/external/github.com/llvm/llvm-project/+log/084e2b53..ecea8371

https://chromium.googlesource.com/external/github.com/rust-lang/rust/+log/32dd3795bce8..3cf924b93432

Also update clang-cl flags to pass "/clang:-O2" after /O2
after  llvm/llvm-project#95406

Ran:
./tools/clang/scripts/upload_revision.py ecea8371ff03c15fb3dc27ee4108b98335fd2d63
tools/clang/scripts/sync_deps.py
tools/rust/gnrt_stdlib.py
third_party/abseil-cpp/generate_def_files.py

Bug: 340924169
Change-Id: Ie3680f33261952f66d972398656661090c47726a
Disable-Rts: True
Binary-Size: Inlining heuristic change, see crbug.com/347984812.
Cq-Include-Trybots: chromium/try:chromeos-amd64-generic-cfi-thin-lto-rel
Cq-Include-Trybots: chromium/try:dawn-win10-x86-deps-rel
Cq-Include-Trybots: chromium/try:lacros-arm64-generic-rel
Cq-Include-Trybots: chromium/try:linux-chromeos-dbg
Cq-Include-Trybots: chromium/try:linux_chromium_cfi_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_chromeos_msan_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_msan_rel_ng
Cq-Include-Trybots: chromium/try:mac11-arm64-rel,mac_chromium_asan_rel_ng
Cq-Include-Trybots: chromium/try:ios-catalyst,win-asan,android-official
Cq-Include-Trybots: chromium/try:fuchsia-arm64-cast-receiver-rel
Cq-Include-Trybots: chromium/try:mac-official,linux-official
Cq-Include-Trybots: chromium/try:win-official,win32-official
Cq-Include-Trybots: chromium/try:win-arm64-rel
Cq-Include-Trybots: chromium/try:linux-swangle-try-x64,win-swangle-try-x86
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-rel
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-rel
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-rel
Cq-Include-Trybots: chrome/try:iphone-device,ipad-device
Cq-Include-Trybots: chrome/try:linux-chromeos-chrome
Cq-Include-Trybots: chrome/try:win-chrome,win64-chrome,linux-chrome,mac-chrome
Cq-Include-Trybots: chrome/try:linux-pgo,mac-pgo,win32-pgo,win64-pgo
Cq-Include-Trybots: chromium/try:android-rust-arm32-rel
Cq-Include-Trybots: chromium/try:android-rust-arm64-dbg
Cq-Include-Trybots: chromium/try:android-rust-arm64-rel
Cq-Include-Trybots: chromium/try:linux-rust-x64-dbg
Cq-Include-Trybots: chromium/try:linux-rust-x64-rel
Cq-Include-Trybots: chromium/try:mac-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5633975
Reviewed-by: Nico Weber <[email protected]>
Commit-Queue: Hans Wennborg <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317359}
NOKEYCHECK=True
GitOrigin-RevId: 65d491f36bac46186a87e0cc890bb810dc66c4f1
EthanLuisMcDonough pushed a commit to EthanLuisMcDonough/llvm-project that referenced this pull request Aug 13, 2024
/Ot (which is also implied by /O2) is supposed to optimize for maximum
speed, so -O3 seems like a better match.
oneseer pushed a commit to oneseer/rust that referenced this pull request May 24, 2025
…14561-gecea8371-1 / 32dd3795bce8b347fda786529cf5e42a813e0b7d-2 : 3cf924b934322fd7b514600a7dc84fc517515346-1

https://chromium.googlesource.com/external/github.com/llvm/llvm-project/+log/084e2b53..ecea8371

https://chromium.googlesource.com/external/github.com/rust-lang/rust/+log/32dd3795bce8..3cf924b93432

Also update clang-cl flags to pass "/clang:-O2" after /O2
after  llvm/llvm-project#95406

Ran:
./tools/clang/scripts/upload_revision.py ecea8371ff03c15fb3dc27ee4108b98335fd2d63
tools/clang/scripts/sync_deps.py
tools/rust/gnrt_stdlib.py
third_party/abseil-cpp/generate_def_files.py

Bug: 340924169
Change-Id: Ie3680f33261952f66d972398656661090c47726a
Disable-Rts: True
Binary-Size: Inlining heuristic change, see crbug.com/347984812.
Cq-Include-Trybots: chromium/try:chromeos-amd64-generic-cfi-thin-lto-rel
Cq-Include-Trybots: chromium/try:dawn-win10-x86-deps-rel
Cq-Include-Trybots: chromium/try:lacros-arm64-generic-rel
Cq-Include-Trybots: chromium/try:linux-chromeos-dbg
Cq-Include-Trybots: chromium/try:linux_chromium_cfi_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_chromeos_msan_rel_ng
Cq-Include-Trybots: chromium/try:linux_chromium_msan_rel_ng
Cq-Include-Trybots: chromium/try:mac11-arm64-rel,mac_chromium_asan_rel_ng
Cq-Include-Trybots: chromium/try:ios-catalyst,win-asan,android-official
Cq-Include-Trybots: chromium/try:fuchsia-arm64-cast-receiver-rel
Cq-Include-Trybots: chromium/try:mac-official,linux-official
Cq-Include-Trybots: chromium/try:win-official,win32-official
Cq-Include-Trybots: chromium/try:win-arm64-rel
Cq-Include-Trybots: chromium/try:linux-swangle-try-x64,win-swangle-try-x86
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-arm64-rel
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-dbg
Cq-Include-Trybots: chromium/try:android-cronet-mainline-clang-x86-rel
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-dbg
Cq-Include-Trybots: chromium/try:android-cronet-riscv64-rel
Cq-Include-Trybots: chrome/try:iphone-device,ipad-device
Cq-Include-Trybots: chrome/try:linux-chromeos-chrome
Cq-Include-Trybots: chrome/try:win-chrome,win64-chrome,linux-chrome,mac-chrome
Cq-Include-Trybots: chrome/try:linux-pgo,mac-pgo,win32-pgo,win64-pgo
Cq-Include-Trybots: chromium/try:android-rust-arm32-rel
Cq-Include-Trybots: chromium/try:android-rust-arm64-dbg
Cq-Include-Trybots: chromium/try:android-rust-arm64-rel
Cq-Include-Trybots: chromium/try:linux-rust-x64-dbg
Cq-Include-Trybots: chromium/try:linux-rust-x64-rel
Cq-Include-Trybots: chromium/try:mac-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-dbg
Cq-Include-Trybots: chromium/try:win-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5633975
Reviewed-by: Nico Weber <[email protected]>
Commit-Queue: Hans Wennborg <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317359}
NOKEYCHECK=True
GitOrigin-RevId: 65d491f36bac46186a87e0cc890bb810dc66c4f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category clang-cl `clang-cl` driver. Don't use for other compiler parts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants