Skip to content

[DXIL] Don't generate per-variable guards for DirectX #106096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

pow2clk
Copy link
Contributor

@pow2clk pow2clk commented Aug 26, 2024

Thread init guards are generated for local static variables when using the Microsoft CXX ABI. This ABI is also used for HLSL generation, but DXIL doesn't need the corresponding _Init_thread_header/footer calls and doesn't really have a way to handle them in its output targets.

This modifies the language ops when the target is DXIL to exclude this so that they won't be generated and an alternate guardvar method is used that is compatible with the usage.

Done to facilitate testing for #89806, but isn't really related

Thread init guards are generated for local static variables when
using the Microsoft CXX ABI. This ABI is also used for HLSL generation,
but DXIL doesn't need the corresponding _Init_thread_header/footer
calls and doesn't really have a way to handle them in its output
targets.

This modifies the language ops when the target is DXIL to exclude this
so that they won't be generated and an alternate guardvar method is used.

Done to facilitate testing for llvm#89806, but isn't really related
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" backend:DirectX HLSL HLSL Language Support labels Aug 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 26, 2024

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-backend-directx

@llvm/pr-subscribers-clang

Author: Greg Roth (pow2clk)

Changes

Thread init guards are generated for local static variables when using the Microsoft CXX ABI. This ABI is also used for HLSL generation, but DXIL doesn't need the corresponding _Init_thread_header/footer calls and doesn't really have a way to handle them in its output targets.

This modifies the language ops when the target is DXIL to exclude this so that they won't be generated and an alternate guardvar method is used that is compatible with the usage.

Done to facilitate testing for #89806, but isn't really related


Full diff: https://github.com/llvm/llvm-project/pull/106096.diff

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/DirectX.h (+7)
  • (added) clang/test/CodeGenHLSL/static-local-ctor.hlsl (+37)
diff --git a/clang/lib/Basic/Targets/DirectX.h b/clang/lib/Basic/Targets/DirectX.h
index a084e2823453fc..cf7ea5e83503dc 100644
--- a/clang/lib/Basic/Targets/DirectX.h
+++ b/clang/lib/Basic/Targets/DirectX.h
@@ -94,6 +94,13 @@ class LLVM_LIBRARY_VISIBILITY DirectXTargetInfo : public TargetInfo {
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return TargetInfo::VoidPtrBuiltinVaList;
   }
+
+  void adjust(DiagnosticsEngine &Diags, LangOptions &Opts) override {
+    TargetInfo::adjust(Diags, Opts);
+    // The static values this addresses do not apply outside of the same thread
+    // This protection is neither available nor needed
+    Opts.ThreadsafeStatics = false;
+  }
 };
 
 } // namespace targets
diff --git a/clang/test/CodeGenHLSL/static-local-ctor.hlsl b/clang/test/CodeGenHLSL/static-local-ctor.hlsl
new file mode 100644
index 00000000000000..d19f843b6f25c3
--- /dev/null
+++ b/clang/test/CodeGenHLSL/static-local-ctor.hlsl
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -emit-llvm -o - -disable-llvm-passes %s | FileCheck %s
+
+// Verify that no per variable _Init_thread instructions are emitted for non-trivial static locals
+// These would normally be emitted by the MicrosoftCXXABI, but the DirectX backend should exlude them
+// Instead, check for the guardvar oparations that should protect the constructor initialization should
+// only take place once.
+
+RWBuffer<int> buf[10];
+
+void InitBuf(RWBuffer<int> buf) {
+  for (unsigned i; i < 100; i++)
+    buf[i] = 0;
+}
+
+// CHECK-NOT: _Init_thread_epoch
+// CHECK: define internal void @"?main@@YAXXZ"
+// CHECK-NEXT: entry:
+// CHECK-NEXT: [[Tmp1:%.*]] = alloca %"class.hlsl::RWBuffer"
+// CHECK-NEXT: [[Tmp2:%.*]] = load i32, ptr
+// CHECK-NEXT: [[Tmp3:%.*]] = and i32 [[Tmp2]], 1
+// CHECK-NEXT: [[Tmp4:%.*]] = icmp eq i32 [[Tmp3]], 0
+// CHECK-NEXT: br i1 [[Tmp4]]
+// CHECK-NOT: _Init_thread_header
+// CHECK: init:
+// CHECK-NEXT: = or i32 [[Tmp2]], 1
+// CHECK-NOT: _Init_thread_footer
+
+
+[shader("compute")]
+[numthreads(1,1,1)]
+void main() {
+  // A non-trivially initialized static local will get checks to verify that it is generated just once
+  static RWBuffer<int> mybuf;
+  mybuf = buf[0];
+  InitBuf(mybuf);
+}
+

Copy link
Contributor

@dmpots dmpots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pow2clk pow2clk merged commit 26c582b into llvm:main Aug 28, 2024
8 checks passed
@pow2clk pow2clk deleted the hlsl_no_threadinit branch August 28, 2024 21:08
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 28, 2024

LLVM Buildbot has detected a new failure on builder clang-armv8-quick running on linaro-clang-armv8-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/3471

Here is the relevant piece of the build log for the reference
Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'ClangPseudo :: cxx/empty-member-spec.cpp' FAILED ********************
Exit Code: 2

Command Output (stderr):
--
RUN: at line 1: clang-pseudo -grammar=cxx -source=/home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang-tools-extra/pseudo/test/cxx/empty-member-spec.cpp --print-forest | /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang-tools-extra/pseudo/test/cxx/empty-member-spec.cpp
+ clang-pseudo -grammar=cxx -source=/home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang-tools-extra/pseudo/test/cxx/empty-member-spec.cpp --print-forest
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang-tools-extra/pseudo/test/cxx/empty-member-spec.cpp
clang-pseudo: ../llvm/clang-tools-extra/pseudo/lib/cxx/CXX.cpp:437: auto clang::pseudo::cxx::getLanguage()::(anonymous class)::operator()() const: Assertion `Diags.empty()' failed.
#0 0x0076ed24 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang-pseudo+0x5ed24)
#1 0x0076cab4 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang-pseudo+0x5cab4)
#2 0x0076f778 SignalHandler(int) Signals.cpp:0:0
#3 0xf7b2d6e0 __default_sa_restorer ./signal/../sysdeps/unix/sysv/linux/arm/sigrestorer.S:67:0
#4 0xf7b1db06 ./csu/../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47:0
#5 0xf7b5d292 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
#6 0xf7b2c840 gsignal ./signal/../sysdeps/posix/raise.c:27:6
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang-tools-extra/pseudo/test/cxx/empty-member-spec.cpp

--

********************


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:DirectX clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category HLSL HLSL Language Support
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants