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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
