[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this is getting close -- mostly just nits at this point.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3463
+The ``optimize`` attribute, when attached to a function, indicates that the
+function be compiled with a different optimization level than specified on the
+command line. See the Function Attributes documentation on GCC's docs for more
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3465-3467
+information. Currently, the attribute differs from GCC's in that we only support
+one argument and we don't support "-f" arguments. We also don't support
+expressions and integers as arguments, unlike GCC.
----------------



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1932-1934
+  if (const auto *OA = D->getAttr<OptimizeAttr>()) {
+    HasOptimizeAttrO0 = OA->getOptLevel() == OptimizeAttr::O0;
+  }
----------------
Coding style nit.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4851
+
+  std::unordered_map<std::string, OptimizeAttr::OptLevelKind> StrToKind = {
+      {"", OptimizeAttr::O0},  {"s", OptimizeAttr::Os},
----------------
Given that most of the strings we're dealing with are either literals or a `StringRef`, I think you should use `llvm:: StringMap` instead of `unordered_map`.  (We tend to avoid STL containers because their performance characteristics are often different than what we need as a compiler.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4859
+
+  DEBUG_WITH_TYPE("foo", llvm::dbgs() << "Level: " << Level << "\n");
+
----------------
Probably meant to remove this?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4869
+  if (!Level.getAsInteger(10, Num)) {
+    // Limit level to -O4 if higher
+    std::string Level = std::to_string(Num.getLimitedValue(4));
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4841-4850
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+    Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+    Level = Arg.substr(1);
+  else
----------------
steplong wrote:
> aaron.ballman wrote:
> > Then, in here, you can parse the `-O<whatever>` the user passed as a string, and convert it to an `OptimizeAttr::OptLevelKind` enumerator and store that in the semantic attribute.
> > 
> > This allows you to map things like `-Og` to whatever -O level that actually represents, or do any other kind of mapping that works for you.
> > 
> > One question we should probably figure out is whether we also want to support clang-cl optimization strings or not. e.g., `__attribute__((optimize("/O0")))` with a slash instead of a dash. Since we're already going to be doing parsing from here anyway, I feel like it might not be a bad idea to also support those. FWIW, here's the list from the user's manual:
> > ```
> >   /O0                     Disable optimization
> >   /O1                     Optimize for size  (same as /Og     /Os /Oy /Ob2 /GF /Gy)
> >   /O2                     Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)
> >   /Ob0                    Disable function inlining
> >   /Ob1                    Only inline functions which are (explicitly or implicitly) marked inline
> >   /Ob2                    Inline functions as deemed beneficial by the compiler
> >   /Od                     Disable optimization
> >   /Og                     No effect
> >   /Oi-                    Disable use of builtin functions
> >   /Oi                     Enable use of builtin functions
> >   /Os                     Optimize for size
> >   /Ot                     Optimize for speed
> >   /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)
> >   /O<flags>               Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'
> > ```
> > (Not all of these would be supported, like enable use of builtin functions, etc.) WDYT?
> Hmm I don't think it's necessary to get pragma optimize to work, but it shouldn't be hard to add the parsing logic for some of these strings
Definitely agreed on the MSVC command line switches, so if those are a burden, feel free to skip them. For the other flags, those seem more important to support because they're also flags present in GCC so users are more likely to expect them to work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984