[PATCH] D12087: always_inline codegen rewrite

[Richard Smith via cfe-commits]

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