Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added a comment.

second attempt in r247494


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgenii Stepanov via cfe-commits]

Thanks. I just reproduced it on release w/o asserts build and fixed locally.
The other problem is fixed as well, I'll try re-landing now. I'll keep
an eye on the bot later today and will revert again if necessary.


On Fri, Sep 11, 2015 at 6:02 PM, H.J. Lu  wrote:
> On Fri, Sep 11, 2015 at 4:45 PM, Evgenii Stepanov  wrote:
>> Does it say that there is no entry basic block? I.e. the output
>> apparently looks like
>>
>> define void @h() #1 {
>>   store void ()* @f1, void ()** @p, align 8
>>
>> Could you confirm it? Never seen this behavior.
>>
>> I'm going to revert the change due to this and also one broken gdb
>> test (something wrong with debug info in the code that has been
>> always-inlined twice).
>
> I got
>
> [hjl@gnu-mic-2 llvm-clang]$
> /export/build/gnu/llvm-clang/build-x86_64-linux/./bin/clang  -target
> x86_64-pc-linux-gnu -mllvm -disable-llvm-optzns -emit-llvm -S -o -
> /export/gnu/import/git/llvm/tools/clang/test/CodeGen/always_inline.c
> ; ModuleID = 
> '/export/gnu/import/git/llvm/tools/clang/test/CodeGen/always_inline.c'
> target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> target triple = "x86_64-pc-linux-gnu"
>
> ; Function Attrs: nounwind uwtable
> define i32 @f1() #0 {
>   %1 = call i32 @f0.alwaysinline()
>   ret i32 %1
> }
>
> ; Function Attrs: alwaysinline inlinehint nounwind uwtable
> define internal i32 @f2.alwaysinline() #1 {
>   ret i32 7
> }
>
> ; Function Attrs: inlinehint nounwind uwtable
> define i32 @f2() #2 {
> entry:
>   %0 = musttail call i32 @f2.alwaysinline() #2
>   ret i32 %0
> }
>
> ; Function Attrs: nounwind uwtable
> define i32 @f3() #0 {
>   %1 = call i32 @f2.alwaysinline()
>   ret i32 %1
> }
>
> ; Function Attrs: alwaysinline nounwind uwtable
> define internal i32 @f0.alwaysinline() #3 {
>   ret i32 1
> }
>
> attributes #0 = { nounwind uwtable "disable-tail-calls"="false"
> "less-precise-fpmad"="false" "no-frame-pointer-elim"="true"
> "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false"
> "no-nans-fp-math"="false" "stack-protector-buffer-size"="8"
> "target-cpu"="x86-64" "target-features"="+sse,+sse2"
> "unsafe-fp-math"="false" "use-soft-float"="false" }
> attributes #1 = { alwaysinline inlinehint nounwind uwtable
> "disable-tail-calls"="false" "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64"
> "target-features"="+sse,+sse2" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> attributes #2 = { inlinehint nounwind uwtable
> "disable-tail-calls"="false" "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64"
> "target-features"="+sse,+sse2" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> attributes #3 = { alwaysinline nounwind uwtable
> "disable-tail-calls"="false" "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64"
> "target-features"="+sse,+sse2" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
>
> !llvm.ident = !{!0}
>
> !0 = !{!"clang version 3.8.0 (http://llvm.org/git/clang.git
> 4c682cf50f928f82e286df97d10a3d1fcf68e1a1)
> (http://llvm.org/git/llvm.git
> 125be70dbf9784ef4fab69633afa067c1cceffc9)"}
> [hjl@gnu-mic-2 llvm-clang]$
> [hjl@gnu-mic-2 llvm-clang]$
> /export/build/gnu/llvm-clang/build-x86_64-linux/./bin/clang  -target
> x86_64-pc-linux-gnu -mllvm -disable-llvm-optzns -emit-llvm -S -o -
> /export/gnu/import/git/llvm/tools/clang/test/CodeGen/always_inline.c |
> /export/build/gnu/llvm-clang/build-x86_64-linux/./bin/FileCheck
> /export/gnu/import/git/llvm/tools/clang/test/CodeGen/always_inline.c
> --check-prefix=CHECK-NO-OPTZNS
> /export/gnu/import/git/llvm/tools/clang/test/CodeGen/always_inline.c:26:26:
> error: expected string not found in input
> // CHECK-NO-OPTZNS-NEXT: entry:
>  ^
> :24:18: note: scanning from here
> define i32 @f3() #0 {
>  ^
> :29:12: note: possible intended match here
> ; Function Attrs: alwaysinline nounwind uwtable
>^
> [hjl@gnu-mic-2 llvm-clang]$
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:543
@@ +542,3 @@
+  if (Fn->isUsedByMetadata())
+llvm::ValueAsMetadata::handleRAUW(Fn, StubFn);
+}

As the comment says.
W/o this, the debug info for .alwaysinline instructions is attached to the 
.alwaysinline function, which gets deleted before codegen, and we end up with 
DI as if it is just a declaration (no low/high pc, etc).



Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis updated this revision to Diff 34610.
eugenis added a comment.

Fixed the debug info problem.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087

Files:
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/always-inline.c
  test/CodeGen/always_inline-unused.c
  test/CodeGen/always_inline-wrappers.c
  test/CodeGen/always_inline.c
  test/CodeGen/function-attributes.c
  test/CodeGen/pr9614.c
  test/CodeGenCXX/alwaysinline.cpp
  test/Frontend/optimization-remark-line-directive.c
  test/Frontend/optimization-remark.c
  test/Modules/cxx-irgen.cpp

Index: test/Modules/cxx-irgen.cpp
===
--- test/Modules/cxx-irgen.cpp
+++ test/Modules/cxx-irgen.cpp
@@ -26,14 +26,14 @@
   };
 }
 
-// CHECK-DAG: define available_externally hidden {{.*}}{{signext i32|i32}} @_ZN1SIiE1gEv({{.*}} #[[ALWAYS_INLINE:.*]] align
+// CHECK-DAG: define internal i32 @_ZN1SIiE1gEv.alwaysinline() #[[ALWAYS_INLINE:.*]] align
 int a = S::g();
 
 int b = h();
 
 // CHECK-DAG: define linkonce_odr {{.*}}{{signext i32|i32}} @_Z3minIiET_S0_S0_(i32
 int c = min(1, 2);
-// CHECK: define available_externally {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv({{.*}} #[[ALWAYS_INLINE]] align
+// CHECK-DAG: define internal {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv.alwaysinline() #[[ALWAYS_INLINE]] align
 
 namespace ImplicitSpecialMembers {
   // CHECK-LABEL: define {{.*}} @_ZN22ImplicitSpecialMembers1BC2ERKS0_(
Index: test/Frontend/optimization-remark.c
===
--- test/Frontend/optimization-remark.c
+++ test/Frontend/optimization-remark.c
@@ -32,6 +32,8 @@
 // CHECK-NOT: !llvm.dbg.cu = !{
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+2 {{foo.alwaysinline should always be inlined}}
+// expected-remark@+1 {{foo.alwaysinline inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
 float foz(int x, int y) __attribute__((noinline));
@@ -45,7 +47,7 @@
 // expected-remark@+5 {{foz will not be inlined into bar}}
 // expected-remark@+4 {{foz should never be inlined}}
 // expected-remark@+3 {{foz will not be inlined into bar}}
-// expected-remark@+2 {{foo should always be inlined}}
-// expected-remark@+1 {{foo inlined into bar}}
+// expected-remark@+2 {{foo.alwaysinline should always be inlined}}
+// expected-remark@+1 {{foo.alwaysinline inlined into bar}}
   return foo(j, j - 2) * foz(j - 2, j);
 }
Index: test/Frontend/optimization-remark-line-directive.c
===
--- test/Frontend/optimization-remark-line-directive.c
+++ test/Frontend/optimization-remark-line-directive.c
@@ -5,8 +5,9 @@
 // RUN: %clang_cc1 %s -Rpass=inline -gline-tables-only -dwarf-column-info -emit-llvm-only -verify
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+1 {{foo.alwaysinline inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
-// expected-remark@+2 {{foo inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
+// expected-remark@+2 {{foo.alwaysinline inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
 #line 1230 "/bad/path/to/original.c"
 int bar(int j) { return foo(j, j - 2); }
Index: test/CodeGenCXX/alwaysinline.cpp
===
--- /dev/null
+++ test/CodeGenCXX/alwaysinline.cpp
@@ -0,0 +1,68 @@
+// Test different kinds of alwaysinline *structor definitions.
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+struct A1 {
+  __attribute__((__always_inline__)) A1() {}
+  __attribute__((__always_inline__)) ~A1() {}
+};
+
+void g1() {
+  A1 a1;
+}
+
+struct A2 {
+  inline __attribute__((__always_inline__)) A2() {}
+  inline __attribute__((__always_inline__)) ~A2() {}
+};
+
+void g2() {
+  A2 a2;
+}
+
+struct A3 {
+  inline __attribute__((gnu_inline, __always_inline__)) A3() {}
+  inline __attribute__((gnu_inline, __always_inline__)) ~A3() {}
+};
+
+void g3() {
+  A3 a3;
+}
+
+// CHECK-DAG: define internal void @_ZN2A1C1Ev.alwaysinline(%struct.A1* %this) unnamed_addr #[[AI:[01-9]+]]
+// CHECK-DAG: define internal void @_ZN2A1C2Ev.alwaysinline(%struct.A1* %this) unnamed_addr #[[AI]]
+// CHECK-DAG: define 

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added a comment.

I'm going to commit this tomorrow unless someone speaks up.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/CodeGen/CodeGenModule.h:505
@@ +504,3 @@
+
+  llvm::SetVector AlwaysInlineFunctions;
+

Did you try making this a vector? It'd be nice to avoid the set overhead here 
if we can.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added a comment.

In http://reviews.llvm.org/D12087#239243, @yaron.keren wrote:

> In CGCXX.cpp, may be fixable after this commit:
>
>   // FIXME: An extern template instantiation will create functions with
>   // linkage "AvailableExternally". In libc++, some classes also define
>   // members with attribute "AlwaysInline" and expect no reference to
>   // be generated. It is desirable to reenable this optimisation after
>   // corresponding LLVM changes.


I've removed the checks for alwaysinline and available_externally. Tests + 
bootstrap do not show any new failures.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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, );
+  // 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, );
+  // 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 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 

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis marked 2 inline comments as done.
eugenis added a comment.

Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:469-470
@@ +468,4 @@
+  llvm::LLVMContext  = getModule().getContext();
+  llvm::Function *StubFn =
+  llvm::Function::Create(FT, Fn->getLinkage(), Name, ());
+  assert(StubFn->getName() == Name && "name was uniqued!");

rnk wrote:
> This is a lot of work to do for every always_inline function that got called. 
> Can we do this like:
> 1. Build SmallVector of all non-direct call uses of Fn
> 2. Return if there are no such uses
> 3. Build the stub function replacement
> 4. `for (Use *U : IndirectUses) U->set(StubFn)`
This is a very good idea. It's not exactly as easy as that, but it works, see 
the new code.


Comment at: test/CodeGen/2008-05-19-AlwaysInline.c:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - | not grep sabrina
+// RUN: %clang_cc1 %s -emit-llvm -o - | not grep 'call.*sabrina('
 

rnk wrote:
> FileCheck?
Actually, this chunk is not needed with your proposed change.
Reverted.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Reid Kleckner via cfe-commits]

rnk added a subscriber: rnk.


Comment at: lib/CodeGen/CodeGenModule.cpp:469-470
@@ +468,4 @@
+  llvm::LLVMContext  = getModule().getContext();
+  llvm::Function *StubFn =
+  llvm::Function::Create(FT, Fn->getLinkage(), Name, ());
+  assert(StubFn->getName() == Name && "name was uniqued!");

This is a lot of work to do for every always_inline function that got called. 
Can we do this like:
1. Build SmallVector of all non-direct call uses of Fn
2. Return if there are no such uses
3. Build the stub function replacement
4. `for (Use *U : IndirectUses) U->set(StubFn)`


Comment at: test/CodeGen/2008-05-19-AlwaysInline.c:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - | not grep sabrina
+// RUN: %clang_cc1 %s -emit-llvm -o - | not grep 'call.*sabrina('
 

FileCheck?


Comment at: test/CodeGenCXX/dllimport.cpp:247-250
@@ -246,1 +246,6 @@
 // MSC2-NOT: @"\01?alwaysInline@@YAXXZ"()
+// MSC2: declare dllimport void @"\01?alwaysInline@@YAXXZ"()
+// MSC2-NOT: @"\01?alwaysInline@@YAXXZ"()
+
+// GNU2-NOT: @_Z12alwaysInlinev()
+// GNU2: define linkonce_odr void @_Z12alwaysInlinev()

This change will go away if you only create the decl when its used indirectly.


http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Chandler Carruth via cfe-commits]

chandlerc added a comment.

FYI, we should send an RFC to llvm-dev about the design change of always_inline 
and make sure folks generally like the IR-level direction as well. We can point 
at this review as an example.

I'm happy to write that up and send it if that's useful?


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:447-448
@@ +446,4 @@
+  C-handleOperandChange(GV, IndirectReplacement, U);
+} else if (!isallvm::InvokeInst(U.getUser()) 
+   !isallvm::CallInst(U.getUser())) {
+  U.set(IndirectReplacement);

Do you need to check which operand of the call or invoke you found? For 
instance:

  void f(void (*)());
  extern inline __attribute__((always_inline, gnu_inline)) void g() {}
  void h() { f(g); } // should not use alwaysinline symbol


Comment at: lib/CodeGen/CodeGenModule.cpp:505-506
@@ +504,4 @@
+void CodeGenModule::RewriteAlwaysInlineFunctions() {
+  for (llvm::Function Fn : TheModule.functions()) {
+if (Fn.hasFnAttribute(llvm::Attribute::AlwaysInline)) {
+  RewriteAlwaysInlineFunction(Fn);

I think it would be better to make a list of `always_inline` functions rather 
than actually marking them as `always_inline` in the IR; that way, you don't 
need to walk all the functions here, and you don't need to temporarily violate 
the invariant that only `internal`/`private` functions are ever `always_inline`.


Repository:
  rL LLVM

http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis removed rL LLVM as the repository for this revision.
eugenis updated this revision to Diff 32330.

http://reviews.llvm.org/D12087

Files:
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/2008-05-19-AlwaysInline.c
  test/CodeGen/always-inline.c
  test/CodeGen/always_inline.c
  test/CodeGen/alwaysinline.c
  test/CodeGen/dllimport.c
  test/CodeGen/function-attributes.c
  test/CodeGen/pr9614.c
  test/CodeGenCXX/alwaysinline.cpp
  test/CodeGenCXX/dllimport.cpp
  test/Frontend/optimization-remark-line-directive.c
  test/Frontend/optimization-remark.c
  test/Modules/cxx-irgen.cpp

Index: test/Modules/cxx-irgen.cpp
===
--- test/Modules/cxx-irgen.cpp
+++ test/Modules/cxx-irgen.cpp
@@ -26,14 +26,17 @@
   };
 }
 
-// CHECK-DAG: define available_externally hidden {{.*}}{{signext i32|i32}} @_ZN1SIiE1gEv({{.*}} #[[ALWAYS_INLINE:.*]] align
+// CHECK-DAG: define internal i32 @_ZN1SIiE1gEv.inlinefunction() #[[ALWAYS_INLINE:.*]] align
+// CHECK-DAG: declare hidden i32 @_ZN1SIiE1gEv()
 int a = Sint::g();
 
 int b = h();
 
 // CHECK-DAG: define linkonce_odr {{.*}}{{signext i32|i32}} @_Z3minIiET_S0_S0_(i32
 int c = min(1, 2);
-// CHECK: define available_externally {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv({{.*}} #[[ALWAYS_INLINE]] align
+// CHECK-DAG: define internal {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv.inlinefunction() #[[ALWAYS_INLINE]] align
+// CHECK-DAG: declare {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv()
+
 
 namespace ImplicitSpecialMembers {
   // CHECK-LABEL: define {{.*}} @_ZN22ImplicitSpecialMembers1BC2ERKS0_(
Index: test/Frontend/optimization-remark.c
===
--- test/Frontend/optimization-remark.c
+++ test/Frontend/optimization-remark.c
@@ -32,6 +32,8 @@
 // CHECK-NOT: !llvm.dbg.cu = !{
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+2 {{foo.inlinefunction should always be inlined}}
+// expected-remark@+1 {{foo.inlinefunction inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
 float foz(int x, int y) __attribute__((noinline));
@@ -45,7 +47,7 @@
 // expected-remark@+5 {{foz will not be inlined into bar}}
 // expected-remark@+4 {{foz should never be inlined}}
 // expected-remark@+3 {{foz will not be inlined into bar}}
-// expected-remark@+2 {{foo should always be inlined}}
-// expected-remark@+1 {{foo inlined into bar}}
+// expected-remark@+2 {{foo.inlinefunction should always be inlined}}
+// expected-remark@+1 {{foo.inlinefunction inlined into bar}}
   return foo(j, j - 2) * foz(j - 2, j);
 }
Index: test/Frontend/optimization-remark-line-directive.c
===
--- test/Frontend/optimization-remark-line-directive.c
+++ test/Frontend/optimization-remark-line-directive.c
@@ -5,8 +5,9 @@
 // RUN: %clang_cc1 %s -Rpass=inline -gline-tables-only -dwarf-column-info -emit-llvm-only -verify
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+1 {{foo.inlinefunction inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
-// expected-remark@+2 {{foo inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
+// expected-remark@+2 {{foo.inlinefunction inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
 #line 1230 /bad/path/to/original.c
 int bar(int j) { return foo(j, j - 2); }
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -244,6 +244,11 @@
 USE(noinline)
 
 // MSC2-NOT: @\01?alwaysInline@@YAXXZ()
+// MSC2: declare dllimport void @\01?alwaysInline@@YAXXZ()
+// MSC2-NOT: @\01?alwaysInline@@YAXXZ()
+
+// GNU2-NOT: @_Z12alwaysInlinev()
+// GNU2: define linkonce_odr void @_Z12alwaysInlinev()
 // GNU2-NOT: @_Z12alwaysInlinev()
 __declspec(dllimport) __attribute__((always_inline)) inline void alwaysInline() {}
 USE(alwaysInline)
Index: test/CodeGenCXX/alwaysinline.cpp
===
--- test/CodeGenCXX/alwaysinline.cpp
+++ test/CodeGenCXX/alwaysinline.cpp
@@ -0,0 +1,83 @@
+// Test different kinds of alwaysinline *structor definitions.
+
+// RUN: %clang_cc1 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+// RUN: %clang_cc1 -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+struct A1 {
+  __attribute__((__always_inline__)) A1() {}
+  __attribute__((__always_inline__)) 

Re: [PATCH] D12087: always_inline codegen rewrite

[Richard Smith via cfe-commits]

On Mon, Aug 17, 2015 at 1:59 PM, David Blaikie dblai...@gmail.com wrote:



 On Mon, Aug 17, 2015 at 11:07 AM, Evgeniy Stepanov via cfe-commits 
 cfe-commits@lists.llvm.org wrote:

 eugenis created this revision.
 eugenis added reviewers: chandlerc, rsmith.
 eugenis added a subscriber: cfe-commits.
 eugenis set the repository for this revision to rL LLVM.

 Currently always_inline definitions are emitted as (in most cases) an
 available_externally llvm function with an alwaysinline attribute.


 Naive dude missing some context here: How can that ^ be correct?

 A basic test case doesn't seem to indicate that that's the case:

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

 Compile with LLVM optimizations disabled, etc - the bitcode has no mention
 of available_externally in it...


1) __attribute__((always_inline)) does not imply inline; you need to add
'inline' to f1 or you're not in the in most cases case
2) always-inlining is performed even at -O0; you need -mllvm
-disable-llvm-optzns to see the always_inline body
3) The in most cases case is for C or for C++ functions with
__attribute__((gnu_inline)); in plain C++ you get a linkonce_odr function
definition instead.


 This is not exactly right: always_inline functions are NOT available
 externally, and, for example, libc++ uses this semantics to preserve ABI
 stability.

 Emitting an undefined symbol for an always_inline function is always a
 bug. The current code can still do it in certain cases.
  a. Inliner is an SCC pass. It traverses the graph starting from the
 roots, which are either main() function, or all externally-visible
 functions. Inlining does not happen in functions that are not reachable.
  b. Dead code elimination is not perfect. There are cases where a
 function will become unreachable due to some late optimizations and will
 still be emitted into the binary.

 This patch changes the way always_inline functions are emitted in the
 Clang codegen to ensure this never happens. A function F is emitted as a
 pair of
  a. internal F.inlinefunction() alwaysinline { **original function body**
 }
 and, depending on the function visibility, either
  b1. declare F()
 or
  b2. define external F() { musttail call F.inlinefunction() }

 Frontend ensures that all direct calls go to F.inlinefunction().

 This provides a simple invariant that all alwaysinline functions are
 internal, which can be checked in the IR verifier. Another invariant would
 be that alwaysinline functions never reach the backend.

 This patch is based on ideas by Chandler Carruth and Richard Smith.


 Repository:
   rL LLVM

 http://reviews.llvm.org/D12087

 Files:
   lib/CodeGen/CGCXX.cpp
   lib/CodeGen/CodeGenModule.cpp
   lib/CodeGen/CodeGenModule.h
   lib/CodeGen/ItaniumCXXABI.cpp
   test/CodeGen/2008-05-19-AlwaysInline.c
   test/CodeGen/always-inline.c
   test/CodeGen/always_inline.c
   test/CodeGen/alwaysinline.c
   test/CodeGen/dllimport.c
   test/CodeGen/function-attributes.c
   test/CodeGen/pr9614.c
   test/CodeGenCXX/alwaysinline.cpp
   test/CodeGenCXX/dllimport.cpp
   test/Frontend/optimization-remark-line-directive.c
   test/Frontend/optimization-remark.c
   test/Modules/cxx-irgen.cpp


 ___
 cfe-commits mailing list
 cfe-commits@lists.llvm.org
 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis marked an inline comment as done.


Comment at: lib/CodeGen/CodeGenModule.cpp:447-448
@@ +446,4 @@
+if (C  !isallvm::GlobalValue(C)) {
+  C-handleOperandChange(GV, IndirectReplacement, U);
+  continue;
+}

Good catch.


Comment at: lib/CodeGen/CodeGenModule.cpp:496
@@ +495,3 @@
+CI-setAttributes(Fn-getAttributes());
+if (FT-getReturnType()-isVoidTy())
+  llvm::ReturnInst::Create(Ctx, BB);

I don't see any difference from the current behavior. All lines inlined from 
F.inlinefunction to F are attrubuted directly to F, and inspecting debug info 
in the binary and IR I don't find any mentions of inlinefunction.


http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:513
@@ +512,3 @@
+void CodeGenModule::RewriteAlwaysInlineFunctions() {
+  for (llvm::Function *Fn : AlwaysInlineFunctions)
+RewriteAlwaysInlineFunction(Fn);

Does it matter that this traversal is nondeterministic? (Is the nondeterminism 
visible in the output IR?) Could you use a vector type for 
`AlwaysInlineFunctions`? (Does the same function ever actually get added to it 
twice?) If not, maybe a `SetVector` would be more appropriate.


http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis updated this revision to Diff 32341.

http://reviews.llvm.org/D12087

Files:
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGen/2008-05-19-AlwaysInline.c
  test/CodeGen/always-inline.c
  test/CodeGen/always_inline.c
  test/CodeGen/alwaysinline.c
  test/CodeGen/dllimport.c
  test/CodeGen/function-attributes.c
  test/CodeGen/pr9614.c
  test/CodeGenCXX/alwaysinline.cpp
  test/CodeGenCXX/dllimport.cpp
  test/Frontend/optimization-remark-line-directive.c
  test/Frontend/optimization-remark.c
  test/Modules/cxx-irgen.cpp

Index: test/Modules/cxx-irgen.cpp
===
--- test/Modules/cxx-irgen.cpp
+++ test/Modules/cxx-irgen.cpp
@@ -26,14 +26,17 @@
   };
 }
 
-// CHECK-DAG: define available_externally hidden {{.*}}{{signext i32|i32}} @_ZN1SIiE1gEv({{.*}} #[[ALWAYS_INLINE:.*]] align
+// CHECK-DAG: define internal i32 @_ZN1SIiE1gEv.inlinefunction() #[[ALWAYS_INLINE:.*]] align
+// CHECK-DAG: declare hidden i32 @_ZN1SIiE1gEv()
 int a = Sint::g();
 
 int b = h();
 
 // CHECK-DAG: define linkonce_odr {{.*}}{{signext i32|i32}} @_Z3minIiET_S0_S0_(i32
 int c = min(1, 2);
-// CHECK: define available_externally {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv({{.*}} #[[ALWAYS_INLINE]] align
+// CHECK-DAG: define internal {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv.inlinefunction() #[[ALWAYS_INLINE]] align
+// CHECK-DAG: declare {{.*}}{{signext i32|i32}} @_ZN1SIiE1fEv()
+
 
 namespace ImplicitSpecialMembers {
   // CHECK-LABEL: define {{.*}} @_ZN22ImplicitSpecialMembers1BC2ERKS0_(
Index: test/Frontend/optimization-remark.c
===
--- test/Frontend/optimization-remark.c
+++ test/Frontend/optimization-remark.c
@@ -32,6 +32,8 @@
 // CHECK-NOT: !llvm.dbg.cu = !{
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+2 {{foo.inlinefunction should always be inlined}}
+// expected-remark@+1 {{foo.inlinefunction inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
 float foz(int x, int y) __attribute__((noinline));
@@ -45,7 +47,7 @@
 // expected-remark@+5 {{foz will not be inlined into bar}}
 // expected-remark@+4 {{foz should never be inlined}}
 // expected-remark@+3 {{foz will not be inlined into bar}}
-// expected-remark@+2 {{foo should always be inlined}}
-// expected-remark@+1 {{foo inlined into bar}}
+// expected-remark@+2 {{foo.inlinefunction should always be inlined}}
+// expected-remark@+1 {{foo.inlinefunction inlined into bar}}
   return foo(j, j - 2) * foz(j - 2, j);
 }
Index: test/Frontend/optimization-remark-line-directive.c
===
--- test/Frontend/optimization-remark-line-directive.c
+++ test/Frontend/optimization-remark-line-directive.c
@@ -5,8 +5,9 @@
 // RUN: %clang_cc1 %s -Rpass=inline -gline-tables-only -dwarf-column-info -emit-llvm-only -verify
 
 int foo(int x, int y) __attribute__((always_inline));
+// expected-remark@+1 {{foo.inlinefunction inlined into foo}}
 int foo(int x, int y) { return x + y; }
 
-// expected-remark@+2 {{foo inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
+// expected-remark@+2 {{foo.inlinefunction inlined into bar}} expected-note@+2 {{could not determine the original source location for /bad/path/to/original.c:1230:25}}
 #line 1230 /bad/path/to/original.c
 int bar(int j) { return foo(j, j - 2); }
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -244,6 +244,11 @@
 USE(noinline)
 
 // MSC2-NOT: @\01?alwaysInline@@YAXXZ()
+// MSC2: declare dllimport void @\01?alwaysInline@@YAXXZ()
+// MSC2-NOT: @\01?alwaysInline@@YAXXZ()
+
+// GNU2-NOT: @_Z12alwaysInlinev()
+// GNU2: define linkonce_odr void @_Z12alwaysInlinev()
 // GNU2-NOT: @_Z12alwaysInlinev()
 __declspec(dllimport) __attribute__((always_inline)) inline void alwaysInline() {}
 USE(alwaysInline)
Index: test/CodeGenCXX/alwaysinline.cpp
===
--- test/CodeGenCXX/alwaysinline.cpp
+++ test/CodeGenCXX/alwaysinline.cpp
@@ -0,0 +1,83 @@
+// Test different kinds of alwaysinline *structor definitions.
+
+// RUN: %clang_cc1 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+// RUN: %clang_cc1 -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -mconstructor-aliases -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CALL
+
+struct A1 {
+  __attribute__((__always_inline__)) A1() {}
+  __attribute__((__always_inline__)) 

Re: [PATCH] D12087: always_inline codegen rewrite

[Evgeniy Stepanov via cfe-commits]

eugenis added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:513
@@ +512,3 @@
+void CodeGenModule::RewriteAlwaysInlineFunctions() {
+  for (llvm::Function *Fn : AlwaysInlineFunctions)
+RewriteAlwaysInlineFunction(Fn);

Done. Should I make it a SmallSetVector? The number of always_inline 
functions seems very unpredictable and usually quite big, so it would not help, 
I guess.


http://reviews.llvm.org/D12087



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12087: always_inline codegen rewrite

[David Blaikie via cfe-commits]

On Mon, Aug 17, 2015 at 11:07 AM, Evgeniy Stepanov via cfe-commits 
cfe-commits@lists.llvm.org wrote:

 eugenis created this revision.
 eugenis added reviewers: chandlerc, rsmith.
 eugenis added a subscriber: cfe-commits.
 eugenis set the repository for this revision to rL LLVM.

 Currently always_inline definitions are emitted as (in most cases) an
 available_externally llvm function with an alwaysinline attribute.


Naive dude missing some context here: How can that ^ be correct?

A basic test case doesn't seem to indicate that that's the case:

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

Compile with LLVM optimizations disabled, etc - the bitcode has no mention
of available_externally in it...



 This is not exactly right: always_inline functions are NOT available
 externally, and, for example, libc++ uses this semantics to preserve ABI
 stability.

 Emitting an undefined symbol for an always_inline function is always a
 bug. The current code can still do it in certain cases.
  a. Inliner is an SCC pass. It traverses the graph starting from the
 roots, which are either main() function, or all externally-visible
 functions. Inlining does not happen in functions that are not reachable.
  b. Dead code elimination is not perfect. There are cases where a function
 will become unreachable due to some late optimizations and will still be
 emitted into the binary.

 This patch changes the way always_inline functions are emitted in the
 Clang codegen to ensure this never happens. A function F is emitted as a
 pair of
  a. internal F.inlinefunction() alwaysinline { **original function body** }
 and, depending on the function visibility, either
  b1. declare F()
 or
  b2. define external F() { musttail call F.inlinefunction() }

 Frontend ensures that all direct calls go to F.inlinefunction().

 This provides a simple invariant that all alwaysinline functions are
 internal, which can be checked in the IR verifier. Another invariant would
 be that alwaysinline functions never reach the backend.

 This patch is based on ideas by Chandler Carruth and Richard Smith.


 Repository:
   rL LLVM

 http://reviews.llvm.org/D12087

 Files:
   lib/CodeGen/CGCXX.cpp
   lib/CodeGen/CodeGenModule.cpp
   lib/CodeGen/CodeGenModule.h
   lib/CodeGen/ItaniumCXXABI.cpp
   test/CodeGen/2008-05-19-AlwaysInline.c
   test/CodeGen/always-inline.c
   test/CodeGen/always_inline.c
   test/CodeGen/alwaysinline.c
   test/CodeGen/dllimport.c
   test/CodeGen/function-attributes.c
   test/CodeGen/pr9614.c
   test/CodeGenCXX/alwaysinline.cpp
   test/CodeGenCXX/dllimport.cpp
   test/Frontend/optimization-remark-line-directive.c
   test/Frontend/optimization-remark.c
   test/Modules/cxx-irgen.cpp


 ___
 cfe-commits mailing list
 cfe-commits@lists.llvm.org
 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits