[HLSL] Remove global ctor/dtor variable for non-lib profile.

[python3kgae]

Summary

After generated call for ctor/dtor for entry, global variable for ctor/dtor are useless.
Remove them for non-lib profiles.
Lib profile still need these in case export function used the global variable which require ctor/dtor.

[python3kgae]

Rebase

[beanz]

We should also have a library test case that verifies that the global_dtors variable is kept.

clang/lib/CodeGen/CGHLSLRuntime.h

50 ↗

(On Diff #461014)

You don't need to pass the triple in here. If you have the module, you have the triple.

[python3kgae]

Add test for dtor in lib.

[beanz]

clang/lib/CodeGen/CGHLSLRuntime.cpp

218

I question whether we should do this early or late. It feels wrong to me to have clang modifying IR. There are places where clang does this sort of thing so it isn't unprecedented, but it feels wrong.

clang/lib/Sema/SemaDeclAttr.cpp

6898 ↗

(On Diff #461271)

This change should be separate. It doesn't directly relate to the change you've described in the description. I understand you need it to use the same test source, but don't group unrelated functional changes.

clang/test/CodeGenHLSL/GlobalDestructorsLib.hlsl

1 ↗

(On Diff #461271)

The source of this test and the test it was copied from are almost identical. It feels very much like they should be the same file with different check prefixes. You can even have them share a check prefix for the things that are common and only have a differing prefix for the things that are different.

[python3kgae]

clang/lib/CodeGen/CGHLSLRuntime.cpp

218

The reason I want to do it is that with the global variable for ctors/dtors, the ctor/dtor functions will not be removed in optimization passes.
As a result, the global variable will have 2 stores ( 1 for the ctor, 1 for the inlined ctor ). That might cause optimizations to go a different path.

Also, we insert the call of dtor/ctors to entry which already did what the global variable for ctor/dtor asked. If the global variable for ctor/dtor is still kept, other backends might call the ctor/dtor again.

clang/lib/Sema/SemaDeclAttr.cpp

6898 ↗

(On Diff #461271)

I'll create another PR for this.

clang/test/CodeGenHLSL/GlobalDestructorsLib.hlsl

1 ↗

(On Diff #461271)

Will merge it.

[beanz]

clang/lib/CodeGen/CGHLSLRuntime.cpp

218

That makes sense, and is a fair reasoning. This makes me wonder if we would benefit from having a target-specific hook to add early IR optimization passes, but that is a whole different discussion...

[python3kgae]

clang/lib/CodeGen/CGHLSLRuntime.cpp

218

And a related discussion.

Maybe we should not insert call of ctor/dtor to entries inside a library for the same reason.

We cannot remove the global variable for ctor/dtor when things need to be initialized for exported functions other than entries.
If we insert call of ctor to entires, there will be multiple stores for the global variable( 1 for the ctor, 1 or more for the inlined ctor).
And other backends might call ctor/dtor again when linking.

[python3kgae]

Rebase for allow SV_GroupIndex in lib profile

[aaron.ballman]

Switching up code reviewers for the codegen questions.

[efriedma]

Why are we using different mechanisms for global constructors in "libraries" vs. other code? If we need a mechanism in LLVM already, we might as well use it all the time?

[python3kgae]

In D133993#3816526, @efriedma wrote:

Why are we using different mechanisms for global constructors in "libraries" vs. other code? If we need a mechanism in LLVM already, we might as well use it all the time?

For library, even though we could inline all constructors for entry functions in the library, the exported functions which is not entry function could also require the constructors to be called in the entry function after linking. So we cannot remove the global variable for the constructors in library.
For non-library cases, there should be only 1 entry function and the constructors are already called in the entry function. Keeping the global variable for the constructors will make the global variable has more than 1 store in some case which will disable some optimization opportunity. Also, backends might call the constructors again because the global variable still exists to mark the constructors.

Then we go to the question of why call constructors in clang codeGen. The reason for that is the old version of DXIL does not allow function calls. Call constructors in clang codeGen could help to remove function calls early. Also, it helps to get better optimization results.

[beanz]

In D133993#3816526, @efriedma wrote:

Why are we using different mechanisms for global constructors in "libraries" vs. other code? If we need a mechanism in LLVM already, we might as well use it all the time?

To elaborate here in a different wording... You can kinda think of HLSL "library" builds a lot like static archive linking in C/C++, so for the most part library shaders just work like C++.

The non-library mode is _very_ different. For non-library builds there is no linker, we emit fully contained executable code from the backend. In non-library code we need to inline global constructors into the entry function, but we don't need to keep the global variables around because we have no dynamic linker or loader, shader execution is much more similar to baremetal targets in that regard.

[efriedma]

It seems like it would be better to put the code in an LLVM IR transform pass, if we can. It separates the concerns more clearly, and it would make life easier for other compilers. (If you need it to run early, see registerPipelineEarlySimplificationEPCallback.)

I won't block the patch on that, though.

[beanz]

@efriedma, I think that's a great suggestion. @python3kgae, if you don't adjust this patch, can you file an issue for that so that we don't lose that feedback?

[python3kgae]

In D133993#3840838, @efriedma wrote:

It seems like it would be better to put the code in an LLVM IR transform pass, if we can. It separates the concerns more clearly, and it would make life easier for other compilers. (If you need it to run early, see registerPipelineEarlySimplificationEPCallback.)

I won't block the patch on that, though.

Thanks for the review.
I like the idea to move code to LLVM IR pass.
I'll commit this PR as is, and create another one to move the code.

clang/lib/CodeGen/CGHLSLRuntime.cpp

for (auto &F : M.functions()) {
     for (auto *Fn : CtorFns)
       B.CreateCall(FunctionCallee(Fn));
 
     // Insert global dtors before the terminator of the last instruction
     B.SetInsertPoint(F.back().getTerminator());
     for (auto *Fn : DtorFns)
       B.CreateCall(FunctionCallee(Fn));
   }
+
+  // No need to keep global ctors/dtors for non-lib profile after call to
+  // ctors/dtors added for entry.
+  Triple T(M.getTargetTriple());
+  if (T.getEnvironment() != Triple::EnvironmentType::Library) {
+    if (auto *GV = M.getNamedGlobal("llvm.global_ctors"))
+      GV->eraseFromParent();
+    if (auto *GV = M.getNamedGlobal("llvm.global_dtors"))
+      GV->eraseFromParent();
+  }
 }

clang/test/CodeGenHLSL/GlobalConstructorFunction.hlsl

 
 __attribute__((destructor)) void call_me_last(void) {
   i = 0;
 }
 
 [numthreads(1,1,1)]
 void main(unsigned GI : SV_GroupIndex) {}
 
+// Make sure global variable for ctors/dtors removed.
+// CHECK-NOT:@llvm.global_ctors
+// CHECK-NOT:@llvm.global_dtors
+
 //CHECK: define void @main()
 //CHECK-NEXT: entry:
 //CHECK-NEXT:   call void @"?call_me_first@@YAXXZ"()
 //CHECK-NEXT:   call void @"?then_call_me@@YAXXZ"()
 //CHECK-NEXT:   %0 = call i32 @llvm.dx.flattened.thread.id.in.group()
 //CHECK-NEXT:   call void @"?main@@YAXI@Z"(i32 %0)
 //CHECK-NEXT:   call void @"?call_me_last@@YAXXZ"(
 //CHECK-NEXT:   ret void

clang/test/CodeGenHLSL/GlobalConstructorLib.hlsl

 // RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
 
+// Make sure global variable for ctors exist for lib profile.
+// CHECK:@llvm.global_ctors
+
 RWBuffer<float> Buffer;
 
 [shader("compute")]
 [numthreads(1,1,1)]
 void FirstEntry() {}
 
 // CHECK: define void @FirstEntry()
 // CHECK-NEXT: entry:

clang/test/CodeGenHLSL/GlobalConstructors.hlsl

 // RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
 
 RWBuffer<float> Buffer;
 
 [numthreads(1,1,1)]
 void main(unsigned GI : SV_GroupIndex) {}
 
+// Make sure global variable for ctors/dtors removed.
+// CHECK-NOT:@llvm.global_ctors
+// CHECK-NOT:@llvm.global_dtors
 //CHECK:      define void @main()
 //CHECK-NEXT: entry:
 //CHECK-NEXT:   call void @_GLOBAL__sub_I_GlobalConstructors.hlsl()
 //CHECK-NEXT:   %0 = call i32 @llvm.dx.flattened.thread.id.in.group()
 //CHECK-NEXT:   call void @"?main@@YAXI@Z"(i32 %0)
 //CHECK-NEXT:   ret void

clang/test/CodeGenHLSL/GlobalDestructors.hlsl

-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -std=hlsl202x -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -std=hlsl202x -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s --check-prefixes=CHECK
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -std=hlsl202x -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s --check-prefixes=CS,CHECK
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -std=hlsl202x -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s --check-prefixes=LIB,CHECK
+
+// Make sure global variable for dtors exist for lib profile.
+// LIB:@llvm.global_dtors
+// Make sure global variable for dtors removed for compute profile.
+// CS-NOT:llvm.global_dtors
 
 struct Tail {
   Tail() {
     add(1);
   }
 
   ~Tail() {
     add(-1);

 
 int Pupper::Count = 0;
 
 [numthreads(1,1,1)]
 void main(unsigned GI : SV_GroupIndex) {
   Wag();
 }
 
+// Make sure global variable for ctors/dtors removed.
+// CHECK-NOT:@llvm.global_ctors
+// CHECK-NOT:@llvm.global_dtors
 //CHECK:      define void @main()
 //CHECK-NEXT: entry:
 //CHECK-NEXT:   call void @_GLOBAL__sub_I_GlobalDestructors.hlsl()
 //CHECK-NEXT:   %0 = call i32 @llvm.dx.flattened.thread.id.in.group()
 //CHECK-NEXT:   call void @"?main@@YAXI@Z"(i32 %0)
 //CHECK-NEXT:   call void @_GLOBAL__D_a()
 //CHECK-NEXT:   ret void
 
 // This is really just a sanity check I needed for myself to verify that
 // function scope static variables also get destroyed properly.
 
 //CHECK: define internal void @_GLOBAL__D_a()
 //CHECK-NEXT: entry:
 //CHECK-NEXT:   call void @"??1Tail@@QAA@XZ"(ptr @"?T@?1??Wag@@YAXXZ@4UTail@@A")
 //CHECK-NEXT:   call void @"??1Pupper@@QAA@XZ"(ptr @"?GlobalPup@@3UPupper@@A")
 //CHECK-NEXT:   ret void