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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2265
+  let Spellings = [GCC<"optimize">];
+  let Args = [StringArgument<"Level">];
+  let Subjects = SubjectList<[Function]>;
----------------
Something along these lines adds a "fake" argument to the attribute. This means the parsed attribute doesn't care about this argument (so users don't supply it when writing the attribute themselves), but the semantic attribute (`OptimizeAttr`) will have a member to track the fake argument value and will require extra information when creating the attribute.

This effectively will add:
```
enum OptLevelKind {
  O0,
  O1,
  O2,
  O3,
  O4
} OptLevel;
```
to the semantic attribute.


================
Comment at: clang/include/clang/Basic/Attr.td:2267
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
+}
----------------
You should add some rudimentary documentation for this, and probably point to the GCC docs for further information.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3019-3020
   "%0 attribute takes at least %1 argument%s1">;
+def err_attribute_only_allowed_with_argument : Error <
+  "argument to '%0' should be %1">;
 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
----------------
I'm not 100% in love with the list of valid options in my suggested wording (I had originally listed the valid values manually and that was not much better).

One thing I think is important is that this be a warning rather than an error. Users will pass "-f" strings here which are supported by GCC; they should just be able to ignore the warning in Clang as being harmless, but if it's an error, the user has to change the function signatures in ways that are kind of annoying.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+    HasOptnone = TargetDecl->hasAttr<OptimizeNoneAttr>() ||
+                 (TargetDecl->hasAttr<OptimizeAttr>() &&
+                  TargetDecl->getAttr<OptimizeAttr>()->getLevel() == "0");
----------------
steplong wrote:
> I don't think this is the most ergonomic way. Let me know if you have a better idea of doing this
I'll sprinkle some comments around about how I'd investigate handling this.

Based on those suggestions, here you would be able to ask for `OptimizeAttr->getOptLevel()` and it will return the mapped enumeration value, which should clean this code up to not require string checking.

Btw, `hasAttr()` followed by `getAttr()` is generally a code smell (same smell as `isa` followed by `cast`). You should switch to logic more like:
```
if (const auto *OA = TargetDecl->getAttr<OptimizeAttr>()) {

}
```
so that we only have to traverse the list of attributes once instead of twice.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2171-2172
+                  TargetDecl->getAttr<OptimizeAttr>()->getLevel() == "0");
+    HasOptsize = TargetDecl->hasAttr<OptimizeAttr>() &&
+                 TargetDecl->getAttr<OptimizeAttr>()->getLevel() == "s";
     if (auto *AllocSize = TargetDecl->getAttr<AllocSizeAttr>()) {
----------------
I think we also need to check for `-Ofast`, `-Oz`, and `-Og` (https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L575)


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126984