rsmith added inline comments. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:485 @@ +484,3 @@ + std::string Name = Fn->getName(); + std::string InlineName = Name + ".inlinefunction"; + Fn->setName(InlineName); ---------------- I have a slight preference for ".alwaysinline" over ".inlinefunction", since we don't do this for all inline functions.
================ Comment at: lib/CodeGen/CodeGenModule.cpp:491-492 @@ +490,4 @@ + FindNonDirectCallUses(Fn, &NonDirectCallUses); + // Do not create the wrapper if there are no non-direct call uses, and we are + // not required to emit an external definition. + if (NonDirectCallUses.empty() && ---------------- Should we check this before we rename the function? ================ Comment at: lib/CodeGen/CodeGenModule.cpp:491-492 @@ +490,4 @@ + FindNonDirectCallUses(Fn, &NonDirectCallUses); + // Do not create the wrapper if there are no non-direct call uses, and we are + // not required to emit an external definition. + if (NonDirectCallUses.empty() && ---------------- rsmith wrote: > Should we check this before we rename the function? Should we also skip this if the function has local linkage, regardless of how it's used? ================ Comment at: lib/CodeGen/CodeGenModule.cpp:494 @@ +493,3 @@ + if (NonDirectCallUses.empty() && + (Fn->hasAvailableExternallyLinkage() || Fn->isDiscardableIfUnused())) + return; ---------------- I would expect `available_externally` globals to be considered discardable if unused. Can you fix that in LLVM rather than here? ================ Comment at: lib/CodeGen/CodeGenModule.cpp:540-541 @@ +539,4 @@ + RewriteAlwaysInlineFunction(Fn); + Fn->addFnAttr(llvm::Attribute::AlwaysInline); + Fn->setLinkage(llvm::GlobalValue::InternalLinkage); + Fn->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass); ---------------- Swap over these two, so we never have a non-`internal` `always_inline` function. ================ Comment at: lib/CodeGen/CodeGenModule.h:505 @@ +504,3 @@ + + llvm::SetVector<llvm::Function*> AlwaysInlineFunctions; + ---------------- You only call `AddAlwaysInlineFunction` when creating a definition, which should happen at most once per function. Do you really need a `SetVector` here, or could you use a `SmallVector` instead? ================ Comment at: test/CodeGen/alwaysinline-unused.c:1 @@ +1,2 @@ +// Test alwaysinline definitions w/o any non-direct-call uses. +// None of the declarations are emitted. Stub are only emitted when the original ---------------- Rename this to always_inline-unused.c for consistency with the existing always_inline.c. ================ Comment at: test/CodeGen/alwaysinline.c:1 @@ +1,2 @@ +// Test different kinds of alwaysinline definitions. + ---------------- Seems weird to have both a test/CodeGen/always_inline.c and a test/CodeGen/alwaysinline.c. Either merge these together or rename this to test/CodeGen/always_inline-wrappers.c or similar. ================ Comment at: test/CodeGen/alwaysinline.c:47-53 @@ +46,9 @@ + +// CHECK-DAG: define internal void @f1.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f2.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f3.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f4.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f5.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f6.inlinefunction() #[[AI:[01-9]+]] +// CHECK-DAG: define internal void @f7.inlinefunction() #[[AI:[01-9]+]] + ---------------- Only the first of these should have the `:[01-9]+`, otherwise you'll overwrite `AI` rather than checking it's the same. Also, use `[0-9]+` instead of `[01-9]+`? ================ Comment at: test/CodeGen/alwaysinline.c:55-59 @@ +54,7 @@ + +// CHECK-DAG: musttail call void @f1.inlinefunction() +// CHECK-DAG: musttail call void @f3.inlinefunction() +// CHECK-DAG: musttail call void @f4.inlinefunction() +// CHECK-DAG: musttail call void @f5.inlinefunction() +// CHECK-DAG: musttail call void @f7.inlinefunction() + ---------------- I don't think we need to split `f3` and `f5` here. ================ Comment at: test/CodeGen/alwaysinline.c:55-67 @@ +54,15 @@ + +// CHECK-DAG: musttail call void @f1.inlinefunction() +// CHECK-DAG: musttail call void @f3.inlinefunction() +// CHECK-DAG: musttail call void @f4.inlinefunction() +// CHECK-DAG: musttail call void @f5.inlinefunction() +// CHECK-DAG: musttail call void @f7.inlinefunction() + +// CHECK-DAG: define void @f1() #[[NOAI:[01-9]+]] +// CHECK-DAG: declare void @f2() #[[NOAI:[01-9]+]] +// CHECK-DAG: define internal void @f3() #[[NOAI:[01-9]+]] +// CHECK-DAG: define void @f4() #[[NOAI:[01-9]+]] +// CHECK-DAG: define internal void @f5() #[[NOAI:[01-9]+]] +// CHECK-DAG: declare hidden void @f6() #[[NOAI:[01-9]+]] +// CHECK-DAG: define hidden void @f7() #[[NOAI:[01-9]+]] + ---------------- rsmith wrote: > I don't think we need to split `f3` and `f5` here. Can you interleave the `define` and `musttail` lines so it's more obvious how they correspond? ================ Comment at: test/CodeGen/alwaysinline.c:61-67 @@ +60,9 @@ + +// CHECK-DAG: define void @f1() #[[NOAI:[01-9]+]] +// CHECK-DAG: declare void @f2() #[[NOAI:[01-9]+]] +// CHECK-DAG: define internal void @f3() #[[NOAI:[01-9]+]] +// CHECK-DAG: define void @f4() #[[NOAI:[01-9]+]] +// CHECK-DAG: define internal void @f5() #[[NOAI:[01-9]+]] +// CHECK-DAG: declare hidden void @f6() #[[NOAI:[01-9]+]] +// CHECK-DAG: define hidden void @f7() #[[NOAI:[01-9]+]] + ---------------- Likewise. Repository: rL LLVM http://reviews.llvm.org/D12087 _______________________________________________ cfe-commits mailing list [email protected] http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
