[DXIL][Analysis] Collect Function properties in Metadata Analysis #105728

bharadwajy · 2024-08-22T20:32:52Z

Basic infrastructure to collect Function properties in Metadata Analysis

Add a SmallVector of entry properties to the metadata information.
Add a structure to represent function properties. Currently numthreads and shader kind properties of shader entry functions are represented.

…adata Analysis - Add a map of function-function properties to the metadata information. - Add a structure to represent various function properties. Currently numthreads property of a compute shader entry function is represented.

llvmbot · 2024-08-22T20:33:22Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-backend-directx

Author: S. Bharadwaj Yadavalli (bharadwajy)

Changes

Basic infrastructure to collect Function properties in Metadata Analysis

Add a map of function-properties to the metadata information.
Add a structure to represent function properties. Currently numthreads property of a compute shader entry function is represented.

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

5 Files Affected:

(modified) llvm/include/llvm/Analysis/DXILMetadataAnalysis.h (+6-1)
(modified) llvm/lib/Analysis/DXILMetadataAnalysis.cpp (+50)
(modified) llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll (+2)
(modified) llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs-val-ver-0.0.ll (+3-1)
(modified) llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs.ll (+2)

diff --git a/llvm/include/llvm/Analysis/DXILMetadataAnalysis.h b/llvm/include/llvm/Analysis/DXILMetadataAnalysis.h
index 0515139c01aee6..6195ac08b0e99c 100644
--- a/llvm/include/llvm/Analysis/DXILMetadataAnalysis.h
+++ b/llvm/include/llvm/Analysis/DXILMetadataAnalysis.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_ANALYSIS_DXILMETADATA_H
 #define LLVM_ANALYSIS_DXILMETADATA_H
 
+#include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/VersionTuple.h"
@@ -19,12 +20,16 @@ namespace llvm {
 
 namespace dxil {
 
+struct FunctionProperties {
+  unsigned NumThreads[3];
+};
+
 struct ModuleMetadataInfo {
   VersionTuple DXILVersion{};
   VersionTuple ShaderModelVersion{};
   Triple::EnvironmentType ShaderStage = Triple::UnknownEnvironment;
   VersionTuple ValidatorVersion{};
-
+  std::unordered_map<Function *, FunctionProperties> FunctionPropertyMap{};
   void print(raw_ostream &OS) const;
 };
 
diff --git a/llvm/lib/Analysis/DXILMetadataAnalysis.cpp b/llvm/lib/Analysis/DXILMetadataAnalysis.cpp
index 192e151565370d..39bd750d69649b 100644
--- a/llvm/lib/Analysis/DXILMetadataAnalysis.cpp
+++ b/llvm/lib/Analysis/DXILMetadataAnalysis.cpp
@@ -8,11 +8,16 @@
 
 #include "llvm/Analysis/DXILMetadataAnalysis.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/ErrorHandling.h"
+#include <memory>
+#include <utility>
 
 #define DEBUG_TYPE "dxil-metadata-analysis"
 
@@ -33,6 +38,37 @@ static ModuleMetadataInfo collectMetadataInfo(Module &M) {
     MMDAI.ValidatorVersion =
         VersionTuple(MajorMD->getZExtValue(), MinorMD->getZExtValue());
   }
+
+  if (MMDAI.ShaderStage == Triple::EnvironmentType::Compute) {
+    // For all HLSL Shader functions
+    for (auto &F : M.functions()) {
+      if (!F.hasFnAttribute("hlsl.shader"))
+        continue;
+
+      // Get numthreads attribute value
+      StringRef NumThreadsStr =
+          F.getFnAttribute("hlsl.numthreads").getValueAsString();
+      SmallVector<StringRef> NumThreadsVec;
+      NumThreadsStr.split(NumThreadsVec, ',');
+      if (NumThreadsVec.size() != 3) {
+        report_fatal_error(Twine(F.getName()) +
+                               ": Invalid numthreads specified",
+                           /* gen_crash_diag */ false);
+      }
+      FunctionProperties EFP;
+      auto Zip =
+          llvm::zip(NumThreadsVec, MutableArrayRef<unsigned>(EFP.NumThreads));
+      for (auto It : Zip) {
+        StringRef Str = std::get<0>(It);
+        APInt V;
+        assert(!Str.getAsInteger(10, V) &&
+               "Failed to parse numthreads components as integer values");
+        unsigned &Num = std::get<1>(It);
+        Num = V.getLimitedValue();
+      }
+      MMDAI.FunctionPropertyMap.emplace(std::make_pair(std::addressof(F), EFP));
+    }
+  }
   return MMDAI;
 }
 
@@ -42,6 +78,20 @@ void ModuleMetadataInfo::print(raw_ostream &OS) const {
   OS << "Shader Stage : " << Triple::getEnvironmentTypeName(ShaderStage)
      << "\n";
   OS << "Validator Version : " << ValidatorVersion.getAsString() << "\n";
+  for (auto MapItem : FunctionPropertyMap) {
+    MapItem.first->getReturnType()->print(OS, false, true);
+    OS << " " << MapItem.first->getName() << "(";
+    FunctionType *FT = MapItem.first->getFunctionType();
+    for (unsigned I = 0, Sz = FT->getNumParams(); I < Sz; ++I) {
+      if (I)
+        OS << ",";
+      FT->getParamType(I)->print(OS);
+    }
+    OS << ")\n";
+    OS << "  NumThreads: " << MapItem.second.NumThreads[0] << ","
+       << MapItem.second.NumThreads[1] << "," << MapItem.second.NumThreads[2]
+       << "\n";
+  }
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll
index 53edcde24b189f..d429b6534e4253 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll
@@ -9,6 +9,8 @@ target triple = "dxil-pc-shadermodel6.8-compute"
 ; ANALYSIS-NEXT: DXIL Version : 1.8
 ; ANALYSIS-NEXT: Shader Stage : compute
 ; ANALYSIS-NEXT: Validator Version : 0
+; ANALYSIS-NEXT: void entry()
+; ANALYSIS-NEXT: NumThreads: 1,2,1
 ; ANALYSIS-EMPTY:
 
 define void @entry() #0 {
diff --git a/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs-val-ver-0.0.ll b/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs-val-ver-0.0.ll
index c9ea6c94e36519..b7e6d2fd3cf432 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs-val-ver-0.0.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs-val-ver-0.0.ll
@@ -19,5 +19,7 @@ attributes #0 = { noinline nounwind "exp-shader"="cs" "hlsl.numthreads"="1,2,1"
 ; ANALYSIS: Shader Model Version : 6.6
 ; ANALYSIS-NEXT: DXIL Version : 1.6
 ; ANALYSIS-NEXT: Shader Stage : compute
-; ANALYSIS-NEXT: Validator Version : 0
+; ANALYSIS-NEXT: Validator Version : 0.0
+; ANALYSIS-NEXT: void entry()
+; ANALYSIS-NEXT:   NumThreads: 1,2,1
 ; ANALYSIS-EMPTY:
diff --git a/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs.ll b/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs.ll
index f5e524562ac1e0..77b52ee3478abe 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/shaderModel-cs.ll
@@ -11,6 +11,8 @@ target triple = "dxil-pc-shadermodel6.6-compute"
 ; ANALYSIS-NEXT: DXIL Version : 1.6
 ; ANALYSIS-NEXT: Shader Stage : compute
 ; ANALYSIS-NEXT: Validator Version : 0
+; ANALYSIS-NEXT: void entry()
+; ANALYSIS-NEXT:   NumThreads: 1,2,1
 ; ANALYSIS-EMPTY:
 
 define void @entry() #0 {

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

…ysis Pass - Record shader kind specified for entry function from hlsl.shader attribute - Record all entry functions irrespective of the target shader profile being compute - Update expected output of tests accordingly

github-actions · 2024-08-27T22:10:03Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bogner

Sorry I didn't notice it when the pass itself went in, but we should really have some testing in llvm/test/Analysis/DXILMetadataAnalysis, not just piggy backing on the DirectX backend tests.

bogner · 2024-08-27T22:21:10Z

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

 struct ModuleMetadataInfo {
  VersionTuple DXILVersion{};
  VersionTuple ShaderModelVersion{};
  Triple::EnvironmentType ShaderStage = Triple::UnknownEnvironment;
  VersionTuple ValidatorVersion{};
-
+  std::unordered_map<Function *, FunctionProperties> FunctionPropertyMap{};


Do we actually need to look up functions in a map for the function properties, or do we just need to be able to iterate through them? It might be nice to wire up the metadata analysis into DXILTranslateMetadata before adding more properties so that we can see how they'll be used.

In any case, std::unordered_map is very expensive as data structures go. Since this mapping is built up first and then queried later, we can probably just use a sorted vector instead of something heavyweight like this. There are some good tips on how to choose data structures for situations like this in the LLVM Programmer's Manual

Do we actually need to look up functions in a map for the function properties, or do we just need to be able to iterate through them? It might be nice to wire up the metadata analysis into DXILTranslateMetadata before adding more properties so that we can see how they'll be used.

Function lookup is employed in an implementation I have as a follow on PR that consumes this information in DXILTranslateMetadata pass - see here.

Changed to MapVector.

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

Move testing of DXILMetadata Analysis to llvm/test/Analysis/DXILMetadataAnalysis from llvm/test/CodeGen/DirectX/Metadata and add a new test

bharadwajy · 2024-08-29T00:25:02Z

Sorry I didn't notice it when the pass itself went in, but we should really have some testing in llvm/test/Analysis/DXILMetadataAnalysis, not just piggy backing on the DirectX backend tests.

Moved testing of DXIL Metadata Analysis out of DirectX backend tests to llvm/test/Analysis/DXILMetadataAnalysis and added an additional test.

bogner

A few more questions and comments. We should have a test with multiple entry points as well.

bogner · 2024-08-30T19:23:18Z

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

 struct ModuleMetadataInfo {
  VersionTuple DXILVersion{};
  VersionTuple ShaderModelVersion{};
  Triple::EnvironmentType ShaderStage = Triple::UnknownEnvironment;
  VersionTuple ValidatorVersion{};
-
+  MapVector<Function *, EntryProperties> EntryPropertyMap;


Why a MapVector? Is the insertion order of these functions interesting to preserve for some reason, or is this just here so that it's easy to make the order that we print them in deterministic?

bogner · 2024-08-30T19:28:02Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

+      if (!llvm::to_integer(NumThreadsVec[0], EFP.NumThreadsX, 10))
+        assert(false && "Failed to parse X component of numthreads");


if (x) assert(false) is generally discouraged. It doesn't make much difference, but it's probably better

[[maybe_unused]] bool Success = llvm::to_integer(NumThreadsVec[0], EFP.NumThreadsX, 10); assert(Success&& "Failed to parse X component of numthreads");

bogner · 2024-08-30T19:28:49Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

+      if (!llvm::to_integer(NumThreadsVec[2], EFP.NumThreadsZ, 10))
+        assert(false && "Failed to parse Z component of numthreads");
+    }
+    MMDAI.EntryPropertyMap.insert(std::make_pair(std::addressof(F), EFP));


Why use std::addressof(F) here rather than just &F? Generally you only need std::addressof in certain template contexts

bogner · 2024-08-30T19:30:36Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

     << "\n";
  OS << "Validator Version : " << ValidatorVersion.getAsString() << "\n";
+  for (auto MapItem : EntryPropertyMap) {


We can use C++17 so you could use structured bindings to clean this up slightly if you want. In any case this should probably be const auto & to avoid copying the items unnecessarily

bharadwajy · 2024-08-30T19:36:26Z

A few more questions and comments. We should have a test with multiple entry points as well.

I've already added one - llvm/test/Analysis/DXILMetadataAnalysis/entry-properties.ll.

Collect Entry properties in a SmallVector instead of in a Mapvector. Use structured binding declaration for printing analysis info.

bogner · 2024-08-30T22:18:45Z

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

@@ -9,6 +9,8 @@
 #ifndef LLVM_ANALYSIS_DXILMETADATA_H
 #define LLVM_ANALYSIS_DXILMETADATA_H

+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/Function.h"


A forward declaration should be sufficient for Function

bogner · 2024-08-30T22:19:10Z

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h

 struct ModuleMetadataInfo {
  VersionTuple DXILVersion{};
  VersionTuple ShaderModelVersion{};
  Triple::EnvironmentType ShaderStage = Triple::UnknownEnvironment;
  VersionTuple ValidatorVersion{};
-
+  SmallVector<EntryProperties, 4> EntryPropertyVec{};


Unless there's a specific argument for 4, it's better to use SmallVector<EntryProperties> which will automatically deduce a reasonable size

bogner · 2024-08-30T22:20:13Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

+#include <memory>
+#include <utility>


I don't think <memory> or <utility> are used any more

bogner · 2024-08-30T22:21:10Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

+      NumThreadsStr.split(NumThreadsVec, ',');
+      assert(NumThreadsVec.size() == 3 && "Invalid numthreads specified");
+      // Read in the three component values of numthreads
+      bool Success = llvm::to_integer(NumThreadsVec[0], EFP.NumThreadsX, 10);


You need a [[maybe_unused]] or a (void)Success so that this doesn't trigger warnings in no-asserts builds

bogner · 2024-08-30T22:22:06Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

     << "\n";
  OS << "Validator Version : " << ValidatorVersion.getAsString() << "\n";
+  for (const auto [Ent, SS, NX, NY, NZ] : EntryPropertyVec) {


I think this should be const auto &[Ent, SS, NX, NY, NZ] to avoid a copy. This is also starting to get a bit unwieldy with this many members in the structured binding.

bogner · 2024-08-30T22:27:36Z

Also make sure to update the PR description - it still mentions a map.

bogner · 2024-08-31T10:38:29Z

llvm/lib/Analysis/DXILMetadataAnalysis.cpp

     << "\n";
  OS << "Validator Version : " << ValidatorVersion.getAsString() << "\n";
+  for (const auto EP : EntryPropertyVec) {


I've already pointed this out a couple of times (#105728 (comment), #105728 (comment)), but please don't copy the value here. It's inefficient, and a const reference will do just fine.

I've already pointed this out a couple of times (#105728 (comment), #105728 (comment)), but please don't copy the value here. It's inefficient, and a const reference will do just fine.

Changed.

… type.

…c52f95eba Local branch amd-gfx 10fc52f Merged main:1ace91f925ad87c3e5eb836ad58fdffe60c4aea6 into amd-gfx:a9dc02e5ed6f Remote branch main 8aa8c05 [DXIL][Analysis] Collect Function properties in Metadata Analysis (llvm#105728)

llvmbot added backend:DirectX llvm:analysis Includes value tracking, cost tables and constant folding labels Aug 22, 2024

bharadwajy linked an issue Aug 22, 2024 that may be closed by this pull request

[DXIL] [Analysis] Add Function properties to DXIL Metadata analysis #102955

Closed

python3kgae reviewed Aug 22, 2024

View reviewed changes

llvm/include/llvm/Analysis/DXILMetadataAnalysis.h Outdated Show resolved Hide resolved

bharadwajy requested a review from python3kgae August 23, 2024 19:21

python3kgae approved these changes Aug 26, 2024

View reviewed changes

clang-format

c067683

bharadwajy requested a review from python3kgae August 27, 2024 22:16

bogner reviewed Aug 27, 2024

View reviewed changes

Address PR feedback

ccb97cc

Move testing of DXILMetadata Analysis to llvm/test/Analysis/DXILMetadataAnalysis from llvm/test/CodeGen/DirectX/Metadata and add a new test

bharadwajy requested a review from bogner August 29, 2024 00:25

python3kgae approved these changes Aug 29, 2024

View reviewed changes

bogner reviewed Aug 30, 2024

View reviewed changes

Address latest PR feedback

b3a5362

Collect Entry properties in a SmallVector instead of in a Mapvector. Use structured binding declaration for printing analysis info.

bogner approved these changes Aug 30, 2024

View reviewed changes

bharadwajy added 2 commits August 30, 2024 21:11

A few more changes from PR feedback.

93b26bc

clang-format change

b54693a

bogner requested changes Aug 31, 2024

View reviewed changes

Change inadvertently mis-typed for-loop item declaration to reference…

804cf7a

… type.

bharadwajy requested a review from bogner August 31, 2024 13:44

bogner approved these changes Aug 31, 2024

View reviewed changes

bharadwajy merged commit 8aa8c05 into llvm:main Aug 31, 2024
8 checks passed

bogner mentioned this pull request Sep 4, 2024

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

Merged

python3kgae mentioned this pull request Sep 4, 2024

[DXIL] [Analysis] Add WavieSize to Function properties in DXIL Metadata analysis #107310

Open

		if (!llvm::to_integer(NumThreadsVec[0], EFP.NumThreadsX, 10))
		assert(false && "Failed to parse X component of numthreads");

[DXIL][Analysis] Collect Function properties in Metadata Analysis #105728

[DXIL][Analysis] Collect Function properties in Metadata Analysis #105728

Uh oh!

Conversation

bharadwajy commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bharadwajy commented Aug 29, 2024

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharadwajy commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogner commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bharadwajy commented Aug 22, 2024 •

edited

Loading

llvmbot commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024 •

edited

Loading