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

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini 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);
----------------
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.


https://reviews.llvm.org/D28404