[PATCH] D28404: IRGen: Add optnone attribute on function during O0

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:760-762
   Fn->removeFnAttr(llvm::Attribute::NoInline);
+  Fn->removeFnAttr(llvm::Attribute::OptimizeNone);
   Fn->addFnAttr(llvm::Attribute::AlwaysInline);
----------------
At point where we are in numerous places doing 3 coupled calls, we should add some routine to do this... Maybe we should have when I added the noinline bit.

I don't have a good idea of where best to do this -- as part of or as an alternative to `SetInternalFunctionAttributes`? Something else?

I'm imagining something like `SetAlwaysInlinedRuntimeFunctionAttributes` or something. Need a clang IRGen person to help push the organization in the right direction.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:892
 
+  // -O0 adds the optnone attribute, except if specific attributes prevents it.
+  bool ShouldAddOptNone =
----------------
attributes prevents -> attributes prevent

ACtually, what do you mean by attributes here? Or should this comment instead go below, where we start to branch on the actual 'hasAttr' calls?

After reading below, I understand better. Maybe:

  // Track whether we need to add the optnone LLVM attribute,
  // starting with the default for this optimization level.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:899-900
 
-    // OptimizeNone implies noinline; we should not be inlining such functions.
+    // OptimizeNone implies noinline; we should not be inlining such
+    // functions.
     B.addAttribute(llvm::Attribute::NoInline);
----------------
Unrelated (and unnecessary) formatting change?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:910-912
     // OptimizeNone wins over OptimizeForSize and MinSize.
     F->removeFnAttr(llvm::Attribute::OptimizeForSize);
     F->removeFnAttr(llvm::Attribute::MinSize);
----------------
Is this still at all correct? Why? it seems pretty confusing especially in conjunction with the code below.


I think this may force you to either:
a) stop early-marking of -Os and -Oz flags with these attributes (early: prior to calling this routine) and handling all of the -O flag synthesized attributes here, or
b) set optnone for -O0 wher ewe set optsize for -Os and friends, and then remove it where necessary here.

I don't have any strong opinion about a vs. b.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:962
+  ShouldAddOptNone &= !D->hasAttr<MinSizeAttr>();
+  ShouldAddOptNone &= !D->hasAttr<ColdAttr>();
+  ShouldAddOptNone &= !F->hasFnAttribute(llvm::Attribute::AlwaysInline);
----------------
why is optnone incompatible with *cold*....


https://reviews.llvm.org/D28404