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

[Chris Bieneman via Phabricator via cfe-commits]

beanz 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) {
----------------
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.


================
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) {
----------------
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.


================
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
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133993