Skip to content

[DirectX] initialize registers properties by calling addRegisterClass and computeRegisterProperties #128818

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 1 commit into from
Feb 27, 2025

Conversation

farzonl
Copy link
Member

@farzonl farzonl commented Feb 26, 2025

This fixes #126784 for the DirectX backend.
This bug was marked critical for DX so it is going to go in first. At least one register class needs to be added via addRegisterClass for RegClassForVT to be valid.
Further for costing information used by loop unroll and other optimizations to be valid we need to call computeRegisterProperties. This change does both of these.

The test cases confirm that we can fetch costing information off of getRegisterInfo and that DirectXTargetLowering maps i32 typed registers to DXILClassRegClass.

… and computeRegisterProperties

This fixes llvm#126784 for the DirectX backend.
This bug was marked critical for DX so it is going to go in first.
At least one register class needs to be added via addRegisterClass for RegClassForVT
to be valid.
Further for costing information used by loop unroll and other
optimizations to be valid we need to call computeRegisterProperties.

This change does both of these. The test cases confirm that we can fetch
costing information off of `getRegisterInfo` and that
`DirectXTargetLowering` maps i32 typed registers to
DXILClassRegClass.
@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-backend-directx

Author: Farzon Lotfi (farzonl)

Changes

This fixes #126784 for the DirectX backend.
This bug was marked critical for DX so it is going to go in first. At least one register class needs to be added via addRegisterClass for RegClassForVT to be valid.
Further for costing information used by loop unroll and other optimizations to be valid we need to call computeRegisterProperties. This change does both of these.

The test cases confirm that we can fetch costing information off of getRegisterInfo and that DirectXTargetLowering maps i32 typed registers to DXILClassRegClass.


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

3 Files Affected:

  • (modified) llvm/lib/Target/DirectX/DirectXTargetMachine.cpp (+4-1)
  • (modified) llvm/unittests/Target/DirectX/CMakeLists.txt (+1)
  • (added) llvm/unittests/Target/DirectX/RegisterCostTests.cpp (+65)
diff --git a/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp b/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
index a76c07f784177..dda650b0f6e15 100644
--- a/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
+++ b/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
@@ -187,4 +187,7 @@ DirectXTargetMachine::getTargetTransformInfo(const Function &F) const {
 
 DirectXTargetLowering::DirectXTargetLowering(const DirectXTargetMachine &TM,
                                              const DirectXSubtarget &STI)
-    : TargetLowering(TM) {}
+    : TargetLowering(TM) {
+  addRegisterClass(MVT::i32, &dxil::DXILClassRegClass);
+  computeRegisterProperties(STI.getRegisterInfo());
+}
diff --git a/llvm/unittests/Target/DirectX/CMakeLists.txt b/llvm/unittests/Target/DirectX/CMakeLists.txt
index fd0d5a0dd52c1..5087727ff9800 100644
--- a/llvm/unittests/Target/DirectX/CMakeLists.txt
+++ b/llvm/unittests/Target/DirectX/CMakeLists.txt
@@ -16,4 +16,5 @@ add_llvm_target_unittest(DirectXTests
   CBufferDataLayoutTests.cpp
   PointerTypeAnalysisTests.cpp
   UniqueResourceFromUseTests.cpp
+  RegisterCostTests.cpp
   )
diff --git a/llvm/unittests/Target/DirectX/RegisterCostTests.cpp b/llvm/unittests/Target/DirectX/RegisterCostTests.cpp
new file mode 100644
index 0000000000000..ebf740ed73e98
--- /dev/null
+++ b/llvm/unittests/Target/DirectX/RegisterCostTests.cpp
@@ -0,0 +1,65 @@
+//===- llvm/unittests/Target/DirectX/RegisterCostTests.cpp ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DirectXInstrInfo.h"
+#include "DirectXTargetLowering.h"
+#include "DirectXTargetMachine.h"
+#include "TargetInfo/DirectXTargetInfo.h"
+#include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/Function.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::dxil;
+
+namespace {
+class RegisterCostTests : public testing::Test {
+protected:
+  DirectXInstrInfo DXInstInfo;
+  DirectXRegisterInfo RI;
+  DirectXTargetLowering *DL;
+
+  virtual void SetUp() {
+    LLVMInitializeDirectXTargetMC();
+    Target T = getTheDirectXTarget();
+    RegisterTargetMachine<DirectXTargetMachine> X(T);
+    Triple TT("dxil-pc-shadermodel6.3-library");
+    StringRef CPU = "";
+    StringRef FS = "";
+    DirectXTargetMachine TM(T, TT, CPU, FS, TargetOptions(), Reloc::Static,
+                            CodeModel::Small, CodeGenOptLevel::Default, false);
+    LLVMContext Context;
+    Function *F =
+        Function::Create(FunctionType::get(Type::getVoidTy(Context), false),
+                         Function::ExternalLinkage, 0);
+    DL = new DirectXTargetLowering(TM, *TM.getSubtargetImpl(*F));
+    delete F;
+  }
+  virtual void TearDown() { delete DL; }
+};
+
+TEST_F(RegisterCostTests, TestRepRegClassForVTSet) {
+  const TargetRegisterClass *RC = DL->getRepRegClassFor(MVT::i32);
+  EXPECT_EQ(&dxil::DXILClassRegClass, RC);
+}
+
+TEST_F(RegisterCostTests, TestTrivialCopyCostGetter) {
+
+  const TargetRegisterClass *RC = DXInstInfo.getRegisterInfo().getRegClass(0);
+  unsigned Cost = RC->getCopyCost();
+  EXPECT_EQ(1u, Cost);
+
+  RC = RI.getRegClass(0);
+  Cost = RC->getCopyCost();
+  EXPECT_EQ(1u, Cost);
+}
+} // namespace

Copy link
Contributor

@joaosaffran joaosaffran left a comment

Choose a reason for hiding this comment

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

LGTM


TEST_F(RegisterCostTests, TestTrivialCopyCostGetter) {

const TargetRegisterClass *RC = DXInstInfo.getRegisterInfo().getRegClass(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

DirectXTargetLowering does not expose DirectXRegisterInfo so just testing it seperately to confirm we can fetch costing data off of it.

@farzonl farzonl merged commit dc764f5 into llvm:main Feb 27, 2025
14 checks passed
@farzonl farzonl deleted the bugfix/issue-126784 branch February 27, 2025 15:36
@llvm-ci
Copy link
Collaborator

llvm-ci commented Feb 27, 2025

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

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

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/38/87' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-15220-38-87.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=87 GTEST_SHARD_INDEX=38 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




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


joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Mar 3, 2025
… and computeRegisterProperties (llvm#128818)

This fixes llvm#126784 for the DirectX backend.
This bug was marked critical for DX so it is going to go in first. At
least one register class needs to be added via `addRegisterClass` for
`RegClassForVT` to be valid.
Further for costing information used by loop unroll and other
optimizations to be valid we need to call `computeRegisterProperties`.
This change does both of these.

The test cases confirm that we can fetch costing information off of
`getRegisterInfo` and that `DirectXTargetLowering` maps `i32` typed
registers to `DXILClassRegClass`.
@damyanp damyanp moved this to Closed in HLSL Support Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

[DirectX] Register properties not initialized, causing UB in passes
5 participants