llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.43k stars 11.75k forks source link

Attributes on a function template definition (but not declaration) ignored if instantiation occurs before definition #86849

Open dwblaikie opened 6 months ago

dwblaikie commented 6 months ago

Given:

void f();
template <bool>
#ifdef ATTR_DECL
__attribute__((noinline))
#endif
void f1();
int main() {
    f1<false>();
}
template <bool>
__attribute__((noinline)) void f1() { f(); }

https://godbolt.org/z/MG4qsxvj1 f1 does not have the noinline attribute in the IR/from Clang's CodeGen - but if ATTR_DECL is defined or main is moved to after f1's definition, the attribute is emitted.

GCC does have the same behavior, so one could argue this is intentional/consistent/correct - but seems a bit off? Curious to get some opinions at least

llvmbot commented 6 months ago

@llvm/issue-subscribers-clang-frontend

Author: David Blaikie (dwblaikie)

Given: ``` void f(); template <bool> #ifdef ATTR_DECL __attribute__((noinline)) #endif void f1(); int main() { f1<false>(); } template <bool> __attribute__((noinline)) void f1() { f(); } ``` https://godbolt.org/z/MG4qsxvj1 `f1` does not have the `noinline` attribute in the IR/from Clang's CodeGen - but if `ATTR_DECL` is defined or `main` is moved to after `f1`'s definition, the attribute is emitted. GCC does have the same behavior, so one could argue this is intentional/consistent/correct - but seems a bit off? Curious to get some opinions at least
llvmbot commented 6 months ago

@llvm/issue-subscribers-clang-codegen

Author: David Blaikie (dwblaikie)

Given: ``` void f(); template <bool> #ifdef ATTR_DECL __attribute__((noinline)) #endif void f1(); int main() { f1<false>(); } template <bool> __attribute__((noinline)) void f1() { f(); } ``` https://godbolt.org/z/MG4qsxvj1 `f1` does not have the `noinline` attribute in the IR/from Clang's CodeGen - but if `ATTR_DECL` is defined or `main` is moved to after `f1`'s definition, the attribute is emitted. GCC does have the same behavior, so one could argue this is intentional/consistent/correct - but seems a bit off? Curious to get some opinions at least
AaronBallman commented 6 months ago

Attributes are typically additive on declarations, so this looks like it's following the usual rules for attributes, right? Consider a non-template case:

int f();
#ifdef ATTR_DECL
__attribute__((warn_unused_result))
#endif
int f1();
int main() {
  f1(); // No warning
}
__attribute__((warn_unused_result)) int f1() { return f(); }

void other() {
  f1(); // Warning
}

https://godbolt.org/z/anPvWeTYn

So I think what's happening (I've not verified though!) is that the instantiation introduces a new declaration based on the primary template, that instantiation has no attribute to instantiate and so the resulting decl is unattributed and that's why it behaves like f1() from my non-templated example.

dwblaikie commented 6 months ago

In the case of warn_unused_result - yeah, I don't think there's any other reasonable interpretation other than to apply the attributes available at the time of use (when the attribute applies to uses)

But in this case (ah, sorry, my bug title might've been overly broad - yes, I'm /particularly/ interested in the noinline attribute and how it applies to Clang's generated IR - so perhaps I could narrow down this bug to "codegen-affecting attributes", or even "attributes that affect the code generation of definitions") it's strange that an instantiation of a function declaration affects the IR generation of the function definition.

dwblaikie commented 6 months ago

A non-template example with the same attribute, though, would be this:

void f1();
void f2() {
  f1();
}
__attribute__((noinline)) void f1() {
}

https://godbolt.org/z/MGWqfPovY

In which case f1 does have the noinline function attribute in the emitted IR.

AaronBallman commented 6 months ago

Ah, thank you for the clarification! Pulling in @erichkeane as well for his opinions as attributes and templates code owner.

We could have codegen look for the attribute only on the definition (as this one is inherited by redeclarations: https://github.com/llvm/llvm-project/blob/5bbc640f64efb7d110559dab132c8d2fb7fbbf37/clang/include/clang/Basic/Attr.td#L1965 and https://github.com/llvm/llvm-project/blob/5bbc640f64efb7d110559dab132c8d2fb7fbbf37/clang/include/clang/Basic/Attr.td#L686).

Also, some attributes are required to be written on the first declaration (https://godbolt.org/z/P49WPn3To) so perhaps that's another idea to explore (though it's likely to break code, so it might not be a great option).

If we change behavior here, we should probably coordinate with GCC to keep things consistent.

erichkeane commented 6 months ago

We kind of take a piece-meal approach as to whether we pick up the attribute from Templates, as sometimes it makes sense (and sometimes you mean just the declaration). I don't see how we could get away with doing something like this automatically.

zygoloid commented 6 months ago

If we don't change behavior here, I think it'd make sense to warn if a codegen-affecting attribute is added to a template after it's already been instantiated, similar to how we warn for:

void f1();
void f2() {
    f1();
}
void f1() {}
__attribute__((noinline)) void f1();