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

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked 6 inline comments as done.
mehdi_amini 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);
----------------
chandlerc wrote:
> 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.
Yes some refactoring of all this custom handling would be welcome. I'll take any pointer to how to do it in clang (I'm not familiar enough with clang).


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:892
 
+  // -O0 adds the optnone attribute, except if specific attributes prevents it.
+  bool ShouldAddOptNone =
----------------
chandlerc wrote:
> 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.
Actually I instead moved it all together.


================
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);
----------------
chandlerc wrote:
> 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.
I believe it is still correct: during Os/Oz we reach this point and figure that there is `__attribute__((optnone))` in the *source* (not `-O0`), we remove the attributes, nothing changes. Did I miss something?



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:962
+  ShouldAddOptNone &= !D->hasAttr<MinSizeAttr>();
+  ShouldAddOptNone &= !D->hasAttr<ColdAttr>();
+  ShouldAddOptNone &= !F->hasFnAttribute(llvm::Attribute::AlwaysInline);
----------------
chandlerc wrote:
> why is optnone incompatible with *cold*....
The source attribute "Cold" adds `llvm::Attribute::OptimizeForSize` even at O0 right now, I changed this and now we emit `optnone` at O0 in this case.


https://reviews.llvm.org/D28404