[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/CodeGenModule.cpp:910-912
     // OptimizeNone wins over OptimizeForSize and MinSize.
     F->removeFnAttr(llvm::Attribute::OptimizeForSize);
     F->removeFnAttr(llvm::Attribute::MinSize);
----------------
mehdi_amini wrote:
> probinson wrote:
> > mehdi_amini wrote:
> > > 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?
> > > 
> > Hmmm the Os/Oz attributes are added in CGCall.cpp, and are guarded with a check on the presence of the Optnone source attribute, so if the Optnone source attribute is present we should never see these.  And Os/Oz set OptimizationLevel to 2, which is not zero, so we won't come through here for ShouldAddOptNone reasons either.
> > Therefore these 'remove' calls should be no-ops and could be removed.  (For paranoia you could turn them into asserts, and do some experimenting to see whether I'm confused about how this all fits together.)
> The verifier is already complaining if we get this wrong, and indeed it complains if I'm removing these.
> See clang/test/CodeGen/attr-func-def.c:
> 
> ```
> 
> int foo1(int);
> 
> int foo2(int a) {
>   return foo1(a + 2);
> }
> 
> __attribute__((optnone))
> int foo1(int a) {
>     return a + 1;
> }
> ```
> 
> Here we have the attributed optnone on the definition but not the declaration, and the check you're mentioning in CGCalls is only applying to the declaration.
This is all still incredibly confusing code.

I think what would make me happy with this is to have a separate section for each mutually exclusive group of LLVM attributes added to the function. so:

  // Add the relevant optimization level to the LLVM function.
  if (...) {
    B.addAttribute(llvm::Attribute::OptNone);
    F.removeFnAttr(llvm::ATtribute::OptForSize);
    ...
  } else if (...) {
    B.addAttribute(llvm::Attribute::OptForSize);
  } else if (...) }
    ...
  }

  // Add the inlining control attributes.
  if (...) {
    <whatever to set NoInline>
  } else if (...) {
    <whatever to set AlwaysInline>
  } else if (...) {
    <whatever to set inlinehint>
  }

  // Add specific semantic attributes such as 'naked' and 'cold'.
  if (D->hasAttr<NakedAttr>()) {
    B.addAttribute(...::Naked);
  }
  if (D->hasAttr<Cold>()) {
    ...
  }

Even though this means testing the Clang-level attributes multiple times, I think it'll be much less confusing to read and update. We're actually already really close. just need to hoist the non-inlining bits of optnone out, sink the naked attribute down, and hoist the cold sizeopt up.



https://reviews.llvm.org/D28404