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

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked 3 inline comments as done.
python3kgae added inline comments.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:208
+  // ctors/dtors added for entry.
+  Triple T(M.getTargetTriple());
+  if (T.getEnvironment() != Triple::EnvironmentType::Library) {
----------------
beanz wrote:
> 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.
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6898
   Triple Target = S.Context.getTargetInfo().getTriple();
-  if (Target.getEnvironment() != Triple::Compute) {
+  if (Target.getEnvironment() != Triple::Compute &&
+      Target.getEnvironment() != Triple::Library) {
----------------
beanz wrote:
> 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.
I'll create another PR for this.


================
Comment at: clang/test/CodeGenHLSL/GlobalDestructorsLib.hlsl:1
-// 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
+
----------------
beanz wrote:
> 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.
Will merge it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133993