[PATCH] D90275: [clang][IR] Add support for leaf attribute

Tue Nov 10 17:33:39 PST 2020

gulfem added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:1435
+  let Spellings = [GCC<"leaf">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> gulfem wrote:
> > aaron.ballman wrote:
> > > gulfem wrote:
> > > > aaron.ballman wrote:
> > > > > gulfem wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > gulfem wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Should this attribute also be supported on things like ObjC method decls or other function-like interfaces?
> > > > > > > > Do I need to do anything else to support this attribute in Objective-C as well?
> > > > > > > > I think we should support it in all the C languages family.
> > > > > > > >I think we should support it in all the C languages family.
> > > > > > > 
> > > > > > > That's already happening automatically -- there's a C and C++ spelling available for it and the attribute doesn't specify that it requires a particular language mode or target.
> > > > > > > 
> > > > > > > > Do I need to do anything else to support this attribute in Objective-C as well?
> > > > > > > You can add multiple subjects to the list here, so you can have this apply to `Function, ObjCMethod` for both of those. Another one to consider is whether this attribute can be written on a block declaration (like a lambda, but with different syntax). Beyond that, it's mostly just documentation, devising the test cases to ensure the ObjC functionality behaves as expected, possibly some codegen changes, etc.
> > > > > > AFAIK, users can specify function attributes in lambda expressions.
> > > > > > Lambda functions can only be accessed/called by the functions in the same translation unit, right?
> > > > > > Leaf attribute does not have any effect on the functions that are defined in the same translation unit.
> > > > > > For this reason, I'm thinking that leaf attribute would not have any effect if they are used in lambda expressions.
> > > > > > Do you agree with me?
> > > > > > AFAIK, users can specify function attributes in lambda expressions.
> > > > > 
> > > > > I always forget that you can do that for declaration attributes using GNU-style syntax...
> > > > > 
> > > > > > Lambda functions can only be accessed/called by the functions in the same translation unit, right?
> > > > > 
> > > > > Not necessarily, you could pass one across TU boundaries like a function pointer, for instance. e.g.,
> > > > > ```
> > > > > // TU1.cpp
> > > > > void foo() {
> > > > >   auto l = []() { ... };
> > > > >   bar(l);
> > > > > }
> > > > > 
> > > > > // TU2.cpp
> > > > > void bar(auto func) {
> > > > >   func();
> > > > > }
> > > > > ```
> > > > > Not necessarily, you could pass one across TU boundaries like a function pointer, for instance. e.g.,
> > > > As I mentioned before, leaf attribute is specifically intended for library functions and I think all the existing usage of leaf attribute is in the library function declarations. For this reason, I think we do not need to support them for lambdas. Is that reasonable?
> > > > 
> > > > For this reason, I think we do not need to support them for lambdas. Is that reasonable?
> > > 
> > > Is this considered a library function?
> > > 
> > > ```
> > > struct S {
> > >   void f(); // Is this a library function?
> > >   void operator()(); // How about this?
> > > };
> > > ```
> > > If the answer is "no", then I think we only need to support `FunctionDecl` and nothing else (not even `ObjCMethodDecl`, which is like a member function for ObjC). If the answer is "yes", then it's not clear to me whether lambdas should or should not be supported given that the attribute on the lambda expression is attached to the function call operator for the lambda declaration.
> > > If the answer is "no", then I think we only need to support `FunctionDecl` and nothing else (not even `ObjCMethodDecl`, which is like a member function for ObjC). If the answer is "yes", then it's not clear to me whether lambdas should or should not be supported given that the attribute on the lambda expression is attached to the function call operator for the lambda declaration.
> > 
> > I see your point @aaron.ballman. I would say the second one is not really a library function.
> > @jdoerfert also suggested to allow leaf attribute only on declarations.
> > I can add FunctionDecl, so we only allow leaf attribute on function declarations, not on function definitions or member functions.
> > Does that sound good to both of you?
> > 
> > 
> > I see your point @aaron.ballman. I would say the second one is not really a library function.
> 
> I feel like either they both are or they both aren't, but it's a question of how this attribute is intended to be used.
> 
> > @jdoerfert also suggested to allow leaf attribute only on declarations.
> > I can add FunctionDecl, so we only allow leaf attribute on function declarations, not on function definitions or member functions.
> > Does that sound good to both of you?
> 
> I've come around to that approach, but `FunctionDecl` represents any declaration of a function, including a definition. So you'll probably want to add a new `SubsetSubject` in `Attr.td` to represent a function declaration that's not a definition (and we could potentially reuse that subject for a few other attributes that can't be written on a definition). You can use `FunctionDecl::isThisDeclarationADefinition()` to distinguish between declarations and definitions.
> I feel like either they both are or they both aren't, but it's a question of how this attribute is intended to be used.
Sorry for being vague, and please let me try to clarify that. 
What I meant was that existing leaf attribute cases are not like the cases that you provided. 
It is used in library function declarations in Fuchsia and other existing use cases.
Are we ok banning this attribute in function definitions in clang even though this behavior is different than other compilers (GCC, ICC, etc.)?
If yes, I will incorporate the changes that you are suggesting.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90275/new/

https://reviews.llvm.org/D90275