[llvm] [llvm][AArch64] Autoupgrade function attributes from Module attributes. (PR #82763)

Nick Desaulniers via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 14 10:15:09 PDT 2024


nickdesaulniers wrote:

Hi @DanielKristofKiss , I think we should back this out again, based on the test case provided in https://github.com/llvm/llvm-project/pull/84494#issuecomment-1996047458. (cc @pranavk @dhoekwater).

I think we need to revisit the design of these function attributes.  In particular function attributes with binary values are problematic. (as in abstractly "foo"="true" or concretely "guarded-control-stack"="true").  For inlining, we'd like to avoid inlining when caller and callee differ in function attribute value. ([What color is your function](https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/) comes to mind).

But, by having the "binary" function value take the form of "foo"="true", you haven't actually created a "binary" function attribute but actually a "ternary" function attribute, because now "foo"="false" and the complete lack of the foo function attribute are distinct in a way that's not clear if they mean the same thing or not. (Using "what color is your function" terminology, what we wanted was red or blue functions. What we got is red, blue, or green functions).  To truly have binary function values, the attribute should simply take the form of "foo" (no `="true"` or `="false"`).  The presence of the attribute on the function is implicitly "true" and the lack of attribute is implicitly "false".

Otherwise, if it's intentional that the function attribute have 3 possible values, we need to revisit the inlining behavior.  Rather than do so trying to fix forward this, I think for the purposes of having fewer backports for the clang-18 branch, it would be better to:
1. back this out.
2. commit the test case from @pranavk linked above.
3. rework the design of the function attributes to be boolean rather than ternary
4. reland this with updated design for the function attributes.

WDYT?

https://github.com/llvm/llvm-project/pull/82763


More information about the llvm-commits mailing list