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);
----------------
chandlerc wrote:
> 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.
>
Since you answer below the example I gave above, I just want to be sure you
understand that the attributes for the *declarations* are not even handled in
the same file right?
The "state machine" is cross TU here, and it seems to me what you're describing
would require some refactoring between CGCall.cpp and CodeGenModule.cpp.
https://reviews.llvm.org/D28404
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
