[DirectX] Infrastructure to collect shader flags for each function #112967

bharadwajy · 2024-10-18T20:00:30Z

Currently, ShaderFlagsAnalysis pass represents various module-level properties as well as function-level properties of a DXIL Module using a single mask. However, one mask per function is needed for accurate computation of shader flags mask, such as for entry function metadata creation.

This change introduces a structure that wraps a sorted vector of function-shader flag mask pairs that represent function properties instead of a single shader flag mask that represents module properties and properties of all functions. The result type of ShaderFlagsAnalysis pass is changed to newly-defined structure type instead of a single shader flags mask.

This allows accurate computation of shader flags of an entry function (and all functions in a library shader) for use during its metadata generation (DXILTranslateMetadata pass) and its feature flags in DX container globals construction (DXContainerGlobals pass) based on the shader flags mask of functions. However, note that the change to implement propagation of such callee-based shader flags mask computation is planned in a follow-on PR. Consequently, this PR changes shader flag mask computation in DXILTranslateMetadata and DXContainerGlobals passes to simply be a union of module flags and shader flags of all functions, thereby retaining the existing effect of using a single shader flag mask.

llvmbot · 2024-10-18T20:01:05Z

@llvm/pr-subscribers-backend-directx

Author: S. Bharadwaj Yadavalli (bharadwajy)

Changes

Currently, ShaderFlagsAnalysis pass represents various module-level properties as well as function-level properties of a DXIL Module using a single mask. However, separate flags to represent module-level properties and function-level properties are needed for accurate computation of shader flags mask, such as for entry function metadata creation.

This change introduces a structure that allows separate representation of

(a) shader flag mask to represent module properties
(b) a map of function to shader flag mask that represent function properties

instead of a single shader flag mask that represents module properties and properties of all function. The result type of ShaderFlagsAnalysis pass is changed to newly-defined structure type instead of a single shader flags mask.

This seperation allows accurate computation of shader flags of an entry function for use during its metadata generation (DXILTranslateMetadata pass) and its feature flags in DX container globals construction (DXContainerGlobals pass) based on the shader flags mask of functions called in entry function. However, note that the change to implement such callee-based shader flags mask computation is planned in a follow-on PR. Consequently, this PR changes shader flag mask computation in DXILTranslateMetadata and DXContainerGlobals passes to simply be a union of module flags and shader flags of all functions, thereby retaining the existing effect of using a single shader flag mask.

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

4 Files Affected:

(modified) llvm/lib/Target/DirectX/DXContainerGlobals.cpp (+10-5)
(modified) llvm/lib/Target/DirectX/DXILShaderFlags.cpp (+32-14)
(modified) llvm/lib/Target/DirectX/DXILShaderFlags.h (+17-9)
(modified) llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp (+28-18)

diff --git a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
index 2c11373504e8c7..c7202cc04c26dc 100644
--- a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
+++ b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
@@ -78,13 +78,18 @@ bool DXContainerGlobals::runOnModule(Module &M) {
 }
 
 GlobalVariable *DXContainerGlobals::getFeatureFlags(Module &M) {
-  const uint64_t FeatureFlags =
-      static_cast<uint64_t>(getAnalysis<ShaderFlagsAnalysisWrapper>()
-                                .getShaderFlags()
-                                .getFeatureFlags());
+  const DXILModuleShaderFlagsInfo &MSFI =
+      getAnalysis<ShaderFlagsAnalysisWrapper>().getShaderFlags();
+  // TODO: Feature flags mask is obtained as a collection of feature flags
+  // of the shader flags of all functions in the module. Need to verify
+  // and modify the computation of feature flags to be used.
+  uint64_t ConsolidatedFeatureFlags = 0;
+  for (const auto &FuncFlags : MSFI.FuncShaderFlagsMap) {
+    ConsolidatedFeatureFlags |= FuncFlags.second.getFeatureFlags();
+  }
 
   Constant *FeatureFlagsConstant =
-      ConstantInt::get(M.getContext(), APInt(64, FeatureFlags));
+      ConstantInt::get(M.getContext(), APInt(64, ConsolidatedFeatureFlags));
   return buildContainerGlobal(M, FeatureFlagsConstant, "dx.sfi0", "SFI0");
 }
 
diff --git a/llvm/lib/Target/DirectX/DXILShaderFlags.cpp b/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
index 9fa137b4c025e1..8c590862008862 100644
--- a/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
+++ b/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
@@ -20,33 +20,41 @@
 using namespace llvm;
 using namespace llvm::dxil;
 
-static void updateFlags(ComputedShaderFlags &Flags, const Instruction &I) {
+static void updateFlags(DXILModuleShaderFlagsInfo &MSFI, const Instruction &I) {
+  ComputedShaderFlags &FSF = MSFI.FuncShaderFlagsMap[I.getFunction()];
   Type *Ty = I.getType();
   if (Ty->isDoubleTy()) {
-    Flags.Doubles = true;
+    FSF.Doubles = true;
     switch (I.getOpcode()) {
     case Instruction::FDiv:
     case Instruction::UIToFP:
     case Instruction::SIToFP:
     case Instruction::FPToUI:
     case Instruction::FPToSI:
-      Flags.DX11_1_DoubleExtensions = true;
+      FSF.DX11_1_DoubleExtensions = true;
       break;
     }
   }
 }
 
-ComputedShaderFlags ComputedShaderFlags::computeFlags(Module &M) {
-  ComputedShaderFlags Flags;
-  for (const auto &F : M)
+static DXILModuleShaderFlagsInfo computeFlags(Module &M) {
+  DXILModuleShaderFlagsInfo MSFI;
+  for (const auto &F : M) {
+    if (F.isDeclaration())
+      continue;
+    if (!MSFI.FuncShaderFlagsMap.contains(&F)) {
+      ComputedShaderFlags CSF{};
+      MSFI.FuncShaderFlagsMap[&F] = CSF;
+    }
     for (const auto &BB : F)
       for (const auto &I : BB)
-        updateFlags(Flags, I);
-  return Flags;
+        updateFlags(MSFI, I);
+  }
+  return MSFI;
 }
 
 void ComputedShaderFlags::print(raw_ostream &OS) const {
-  uint64_t FlagVal = (uint64_t) * this;
+  uint64_t FlagVal = (uint64_t)*this;
   OS << formatv("; Shader Flags Value: {0:x8}\n;\n", FlagVal);
   if (FlagVal == 0)
     return;
@@ -65,15 +73,25 @@ void ComputedShaderFlags::print(raw_ostream &OS) const {
 
 AnalysisKey ShaderFlagsAnalysis::Key;
 
-ComputedShaderFlags ShaderFlagsAnalysis::run(Module &M,
-                                             ModuleAnalysisManager &AM) {
-  return ComputedShaderFlags::computeFlags(M);
+DXILModuleShaderFlagsInfo ShaderFlagsAnalysis::run(Module &M,
+                                                   ModuleAnalysisManager &AM) {
+  return computeFlags(M);
+}
+
+bool ShaderFlagsAnalysisWrapper::runOnModule(Module &M) {
+  MSFI = computeFlags(M);
+  return false;
 }
 
 PreservedAnalyses ShaderFlagsAnalysisPrinter::run(Module &M,
                                                   ModuleAnalysisManager &AM) {
-  ComputedShaderFlags Flags = AM.getResult<ShaderFlagsAnalysis>(M);
-  Flags.print(OS);
+  DXILModuleShaderFlagsInfo Flags = AM.getResult<ShaderFlagsAnalysis>(M);
+  OS << "; Shader Flags mask for Module:\n";
+  Flags.ModuleFlags.print(OS);
+  for (auto SF : Flags.FuncShaderFlagsMap) {
+    OS << "; Shader Flags mash for Function: " << SF.first->getName() << "\n";
+    SF.second.print(OS);
+  }
   return PreservedAnalyses::all();
 }
 
diff --git a/llvm/lib/Target/DirectX/DXILShaderFlags.h b/llvm/lib/Target/DirectX/DXILShaderFlags.h
index 1df7d27de13d3c..6f81ff74384d0c 100644
--- a/llvm/lib/Target/DirectX/DXILShaderFlags.h
+++ b/llvm/lib/Target/DirectX/DXILShaderFlags.h
@@ -14,6 +14,8 @@
 #ifndef LLVM_TARGET_DIRECTX_DXILSHADERFLAGS_H
 #define LLVM_TARGET_DIRECTX_DXILSHADERFLAGS_H
 
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Compiler.h"
@@ -60,11 +62,20 @@ struct ComputedShaderFlags {
     return FeatureFlags;
   }
 
-  static ComputedShaderFlags computeFlags(Module &M);
   void print(raw_ostream &OS = dbgs()) const;
   LLVM_DUMP_METHOD void dump() const { print(); }
 };
 
+using FunctionShaderFlagsMap =
+    SmallDenseMap<Function const *, ComputedShaderFlags>;
+struct DXILModuleShaderFlagsInfo {
+  // Shader Flag mask representing module-level properties
+  ComputedShaderFlags ModuleFlags;
+  // Map representing shader flag mask representing properties of each of the
+  // functions in the module
+  FunctionShaderFlagsMap FuncShaderFlagsMap;
+};
+
 class ShaderFlagsAnalysis : public AnalysisInfoMixin<ShaderFlagsAnalysis> {
   friend AnalysisInfoMixin<ShaderFlagsAnalysis>;
   static AnalysisKey Key;
@@ -72,9 +83,9 @@ class ShaderFlagsAnalysis : public AnalysisInfoMixin<ShaderFlagsAnalysis> {
 public:
   ShaderFlagsAnalysis() = default;
 
-  using Result = ComputedShaderFlags;
+  using Result = DXILModuleShaderFlagsInfo;
 
-  ComputedShaderFlags run(Module &M, ModuleAnalysisManager &AM);
+  DXILModuleShaderFlagsInfo run(Module &M, ModuleAnalysisManager &AM);
 };
 
 /// Printer pass for ShaderFlagsAnalysis results.
@@ -92,19 +103,16 @@ class ShaderFlagsAnalysisPrinter
 /// This is required because the passes that will depend on this are codegen
 /// passes which run through the legacy pass manager.
 class ShaderFlagsAnalysisWrapper : public ModulePass {
-  ComputedShaderFlags Flags;
+  DXILModuleShaderFlagsInfo MSFI;
 
 public:
   static char ID;
 
   ShaderFlagsAnalysisWrapper() : ModulePass(ID) {}
 
-  const ComputedShaderFlags &getShaderFlags() { return Flags; }
+  const DXILModuleShaderFlagsInfo &getShaderFlags() { return MSFI; }
 
-  bool runOnModule(Module &M) override {
-    Flags = ComputedShaderFlags::computeFlags(M);
-    return false;
-  }
+  bool runOnModule(Module &M) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
diff --git a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
index be370e10df6943..2da4fe83a066c2 100644
--- a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
+++ b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
@@ -286,11 +286,6 @@ static MDTuple *emitTopLevelLibraryNode(Module &M, MDNode *RMD,
   MDTuple *Properties = nullptr;
   if (ShaderFlags != 0) {
     SmallVector<Metadata *> MDVals;
-    // FIXME: ShaderFlagsAnalysis pass needs to collect and provide
-    // ShaderFlags for each entry function. Currently, ShaderFlags value
-    // provided by ShaderFlagsAnalysis pass is created by walking *all* the
-    // function instructions of the module. Is it is correct to use this value
-    // for metadata of the empty library entry?
     MDVals.append(
         getTagValueAsMetadata(EntryPropsTag::ShaderFlags, ShaderFlags, Ctx));
     Properties = MDNode::get(Ctx, MDVals);
@@ -302,7 +297,7 @@ static MDTuple *emitTopLevelLibraryNode(Module &M, MDNode *RMD,
 
 static void translateMetadata(Module &M, const DXILResourceMap &DRM,
                               const Resources &MDResources,
-                              const ComputedShaderFlags &ShaderFlags,
+                              const DXILModuleShaderFlagsInfo &ShaderFlags,
                               const ModuleMetadataInfo &MMDI) {
   LLVMContext &Ctx = M.getContext();
   IRBuilder<> IRB(Ctx);
@@ -318,22 +313,37 @@ static void translateMetadata(Module &M, const DXILResourceMap &DRM,
   // See https://github.com/llvm/llvm-project/issues/57928
   MDTuple *Signatures = nullptr;
 
-  if (MMDI.ShaderProfile == Triple::EnvironmentType::Library)
+  if (MMDI.ShaderProfile == Triple::EnvironmentType::Library) {
+    // Create a consolidated shader flag mask of all functions in the library
+    // to be used as shader flags mask value associated with top-level library
+    // entry metadata.
+    uint64_t ConsolidatedMask = ShaderFlags.ModuleFlags;
+    for (const auto &FunFlags : ShaderFlags.FuncShaderFlagsMap) {
+      ConsolidatedMask |= FunFlags.second;
+    }
     EntryFnMDNodes.emplace_back(
-        emitTopLevelLibraryNode(M, ResourceMD, ShaderFlags));
-  else if (MMDI.EntryPropertyVec.size() > 1) {
+        emitTopLevelLibraryNode(M, ResourceMD, ConsolidatedMask));
+  } else if (MMDI.EntryPropertyVec.size() > 1) {
     M.getContext().diagnose(DiagnosticInfoTranslateMD(
         M, "Non-library shader: One and only one entry expected"));
   }
 
   for (const EntryProperties &EntryProp : MMDI.EntryPropertyVec) {
-    // FIXME: ShaderFlagsAnalysis pass needs to collect and provide
-    // ShaderFlags for each entry function. For now, assume shader flags value
-    // of entry functions being compiled for lib_* shader profile viz.,
-    // EntryPro.Entry is 0.
-    uint64_t EntryShaderFlags =
-        (MMDI.ShaderProfile == Triple::EnvironmentType::Library) ? 0
-                                                                 : ShaderFlags;
+    auto FSFIt = ShaderFlags.FuncShaderFlagsMap.find(EntryProp.Entry);
+    if (FSFIt == ShaderFlags.FuncShaderFlagsMap.end()) {
+      M.getContext().diagnose(DiagnosticInfoTranslateMD(
+          M, "Shader Flags of Function '" + Twine(EntryProp.Entry->getName()) +
+                 "' not found"));
+    }
+    // If ShaderProfile is Library, mask is already consolidated in the
+    // top-level library node. Hence it is not emitted.
+    uint64_t EntryShaderFlags = 0;
+    if (MMDI.ShaderProfile != Triple::EnvironmentType::Library) {
+      // TODO: Create a consolidated shader flag mask of all the entry
+      // functions and its callees. The following is correct only if
+      // (*FSIt).first has no call instructions.
+      EntryShaderFlags = (*FSFIt).second | ShaderFlags.ModuleFlags;
+    }
     if (MMDI.ShaderProfile != Triple::EnvironmentType::Library) {
       if (EntryProp.ShaderStage != MMDI.ShaderProfile) {
         M.getContext().diagnose(DiagnosticInfoTranslateMD(
@@ -361,7 +371,7 @@ PreservedAnalyses DXILTranslateMetadata::run(Module &M,
                                              ModuleAnalysisManager &MAM) {
   const DXILResourceMap &DRM = MAM.getResult<DXILResourceAnalysis>(M);
   const dxil::Resources &MDResources = MAM.getResult<DXILResourceMDAnalysis>(M);
-  const ComputedShaderFlags &ShaderFlags =
+  const DXILModuleShaderFlagsInfo &ShaderFlags =
       MAM.getResult<ShaderFlagsAnalysis>(M);
   const dxil::ModuleMetadataInfo MMDI = MAM.getResult<DXILMetadataAnalysis>(M);
 
@@ -393,7 +403,7 @@ class DXILTranslateMetadataLegacy : public ModulePass {
         getAnalysis<DXILResourceWrapperPass>().getResourceMap();
     const dxil::Resources &MDResources =
         getAnalysis<DXILResourceMDWrapper>().getDXILResource();
-    const ComputedShaderFlags &ShaderFlags =
+    const DXILModuleShaderFlagsInfo &ShaderFlags =
         getAnalysis<ShaderFlagsAnalysisWrapper>().getShaderFlags();
     dxil::ModuleMetadataInfo MMDI =
         getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();

github-actions · 2024-10-18T20:04:05Z

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

Currently, ShaderFlagsAnalysis pass represents various module-level properties as well as function-level properties of a DXIL Module using a single mask. However, separate flags to represent module-level properties and function-level properties are needed for accurate computation of shader flags mask, such as for entry function metadata creation. This change introduces a structure that allows separate representation of (a) shader flag mask to represent module properties (b) a map of function to shader flag mask that represent function properties instead of a single shader flag mask that represents module properties and properties of all function. The result type of ShaderFlagsAnalysis pass is changed to newly-defined structure type instead of a single shader flags mask. This seperation allows accurate computation of shader flags of an entry function for use during its metadata generation (DXILTranslateMetadata pass) and its feature flags in DX container globals construction (DXContainerGlobals pass) based on the shader flags mask of functions called in entry function. However, note that the change to implement such callee-based shader flags mask computation is planned in a follow-on PR. Consequently, this PR changes shader flag mask computation in DXILTranslateMetadata and DXContainerGlobals passes to simply be a union of module flags and shader flags of all functions, thereby retaining the existing effect of using a single shader flag mask.

bogner · 2024-10-22T18:27:47Z

We should be able to write tests that the shader flags are correct per-function by running the analysis on some IR and printing the results, which we can then check.

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

pointers and corresponding shader flag masks. This follows the recommendations in LLVM Programmer's Manual as the current usage pattern has distinct phases of insertion of computed shader flags followed by querying. Upon insertion, the Smallvector is sorted and binary search is used for querying. Necessary comparison function of pairs is also implemented. Added a simple DiagnosticInfoShaderFlags for emitting diagnostics. Added tests to verify shader flags masks collected per-function.

bharadwajy · 2024-10-28T15:27:52Z

We should be able to write tests that the shader flags are correct per-function by running the analysis on some IR and printing the results, which we can then check.

Added per-function test.

damyanp

I stopped reading this after seeing the string construction in the comparison function. I'm either missing something big about this change, which means I'd need to review again once I've been corrected, or this PR is going to be somewhat different anyway, so I'll wait until that's resolved before looking at it again.

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

bharadwajy · 2024-10-28T16:37:41Z

I stopped reading this after seeing the string construction in the comparison function. I'm either missing something big about this change, which means I'd need to review again once I've been corrected, or this PR is going to be somewhat different anyway, so I'll wait until that's resolved before looking at it again.

Lexicographic sort of functions by their signatures seemed the appropriate. I looked at using FunctionComparator::compareSignature() but did not see its usage in the sources other than in tests, so wasn't sure if its usage is encouraged or not.

What are the concerns?

damyanp · 2024-10-28T17:57:53Z

What are the concerns?

Have a think about how many strings will be built, and how many memory allocations there'll be, every time sort or lower_bound needs to compare two elements of the vector.

…nature Non-empty Function names are unique in LLVM IR. Update the expected test output accordingly

bharadwajy · 2024-10-28T20:48:43Z

What are the concerns?

Have a think about how many strings will be built, and how many memory allocations there'll be, every time sort or lower_bound needs to compare two elements of the vector.

Changed to compare functions by their names instead of constructing a pseudo-signature. Non-empty Function names are unique in LLVM IR.

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

…unsorted vector Other changes based on latest PR feedback

Delete unused class DiagnosticInfoShaderFlags

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

llvm/lib/Target/DirectX/DXILShaderFlags.h

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions-obj-test.ll

bogner · 2024-10-30T00:10:09Z

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions-obj-test.ll

+; DXC-NEXT:       Doubles:         true
+; DXC-NOT:   {{[A-Za-z]+: +true}}
+; DXC:            DX11_1_DoubleExtensions:         true
+; DXC-NOT:   {{[A-Za-z]+: +true}}
+; DXC:       NextUnusedBit:   false


What does this print that we need this awkward CHECK-NOT of anything that's specifically "true" rather than just a CHECK-NEXT?

The portion of the output being checked is as follows:

... - Name: SFI0 Size: 8 Flags: Doubles: true ComputeShadersPlusRawAndStructuredBuffers: false UAVsAtEveryStage: false Max64UAVs: false MinimumPrecision: false DX11_1_DoubleExtensions: true DX11_1_ShaderExtensions: false LEVEL9ComparisonFiltering: false TiledResources: false StencilRef: false InnerCoverage: false TypedUAVLoadAdditionalFormats: false ROVs: false ViewportAndRTArrayIndexFromAnyShaderFeedingRasterizer: false WaveOps: false Int64Ops: false ViewID: false Barycentrics: false NativeLowPrecision: false ShadingRate: false Raytracing_Tier_1_1: false SamplerFeedback: false AtomicInt64OnTypedResource: false AtomicInt64OnGroupShared: false DerivativesInMeshAndAmpShaders: false ResourceDescriptorHeapIndexing: false SamplerDescriptorHeapIndexing: false RESERVED: false AtomicInt64OnHeapResource: false AdvancedTextureOps: false WriteableMSAATextures: false NextUnusedBit: false ...

This test and the CHECK-NOT line in question (and all others) that already exist(s) appear to check for flags to be not true with only Doubles and DX11_1_DoubleExtensions that are expected to be true.

This doesn't seem like the place to check that all of the other flags are false. Just doing the two checks should be sufficient:

; CHECK: Doubles: true ; CHECK: DX11_1_DoubleExtensions: true

This doesn't seem like the place to check that all of the other flags are false. Just doing the two checks should be sufficient:

; CHECK: Doubles: true ; CHECK: DX11_1_DoubleExtensions: true

Changes made to limited checking to the two flags.

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

- Use CSF.Doubles directly - Remove type-aliasies FuncShaderFlags* - Make ModuleFlags and FunctionFlags private - Delete DXILModuleShaderFlagsInfo::print() - Delete check prefix DXC from test with a single run - Get rid of compare functions - Change order of expected output accordingly in double-extensions.ll - Add extra comments for clarification - Add back DiagnosticInfoShaderFlags - Additional error checks

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

llvm/lib/Target/DirectX/DXILShaderFlags.h

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

Move the functionality of static void updateFlags(...) to private method void DXILModuleShaderFlagsInfo::updateFuctionFlags(...) and that of static DXILModuleShaderFlagsInfo computeFlags(const Module &M) to public method bool DXILModuleShaderFlagsInfo::initialize(const Module &M).

damyanp

Thanks, this is looking good. I think my remaining opens are really nits.

We should look for someone more expert in this area to approve though.

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

llvm/lib/Target/DirectX/DXILShaderFlags.h

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

Delete DXILModuleShaderFlagsInfo::ModuleFlags and track module flags in shader flags mask of each function. Add private field DXILModuleShaderFlagsinfo::CombinedSFMask to represent combined shader flags masks of all functions. Update the value as it is computed per function. Change DXILModuleShaderFlagsInfo::initialize(Module&) to constructor

damyanp · 2024-11-15T23:22:54Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

@@ -120,16 +127,17 @@ class ShaderFlagsAnalysisPrinter
 /// This is required because the passes that will depend on this are codegen
 /// passes which run through the legacy pass manager.
 class ShaderFlagsAnalysisWrapper : public ModulePass {
-  DXILModuleShaderFlagsInfo MSFI;
+  std::unique_ptr<DXILModuleShaderFlagsInfo> MSFI;


Out of interest, did you consider std::optional for this?

+1

I also question if we need the optional. Why was the old pattern of the uninitialized flag structure a problem?

+1

I also question if we need the optional. Why was the old pattern of the uninitialized flag structure a problem?

Reverted to old pattern; changed to use initialize() instead of a constructor to avoid dynamic memory allocation about which concerns were expressed.

damyanp

LGTM, but would look for a review from someone with more domain knowledge than I have.

llvm-beanz · 2024-11-18T20:53:02Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

+  uint64_t getModuleFlags() const {
+    uint64_t ModuleFlags = 0;
+#define DXIL_MODULE_FLAG(DxilModuleBit, FlagName, Str)                         \
+  ModuleFlags |= FlagName ? getMask(DxilModuleBit) : 0ull;


We have this exact expansion inside the operator uint64_t definition above. Should we maybe refactor this so that one uses the other instead of duplicating the expansion?

We have this exact expansion inside the operator uint64_t definition above. Should we maybe refactor this so that one uses the other instead of duplicating the expansion?

Refactored.

llvm-beanz · 2024-11-18T20:56:38Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

@@ -120,16 +127,17 @@ class ShaderFlagsAnalysisPrinter
 /// This is required because the passes that will depend on this are codegen
 /// passes which run through the legacy pass manager.
 class ShaderFlagsAnalysisWrapper : public ModulePass {
-  DXILModuleShaderFlagsInfo MSFI;
+  std::unique_ptr<DXILModuleShaderFlagsInfo> MSFI;


+1

I also question if we need the optional. Why was the old pattern of the uninitialized flag structure a problem?

llvm-beanz · 2024-11-18T20:59:42Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

  void print(raw_ostream &OS = dbgs()) const;
  LLVM_DUMP_METHOD void dump() const { print(); }
 };

+struct DXILModuleShaderFlagsInfo {


nit: This class's full name is llvm::dxil::DXILModuleShaderFlagsInfo, that's a bit of a mouthful. I'm not really sure how much benefit we get from prefixing it with DXIL and suffixing it with Info.

nit: This class's full name is llvm::dxil::DXILModuleShaderFlagsInfo, that's a bit of a mouthful. I'm not really sure how much benefit we get from prefixing it with DXIL and suffixing it with Info.

Changed to ModuleShaderFlags

llvm-beanz · 2024-11-18T21:10:39Z

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

-                                .getFeatureFlags());
+  // TODO: Feature flags mask is obtained as a collection of feature flags
+  // of the shader flags of all functions in the module. Need to verify
+  // and modify the computation of feature flags to be used.


Do we have an issue tracking this?

llvm-beanz · 2024-11-18T21:21:07Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+/// for Shader Flags Analysis pass
+class DiagnosticInfoShaderFlags : public DiagnosticInfo {
+private:
+  const Twine &Msg;


I get really nervous when someone stores a Twine. This effectively means that you must create and destroy this object in a single expression, otherwise the Twine or its attached arguments can go out of scope and you have a memory error.

It seems to me like what you really need is an adapter that converts an llvm::Error to a DiagnosticInfo, so that you can just pass the Error object right through.

We should add a utility to llvm/Support/Error to facilitate that.

Deleted diagnostic. Error reporting simplified to use report_fatal_error() as the anticipated error conditions are not expected to be triggered during analysis of a well-formed module.

llvm-beanz · 2024-11-18T21:27:32Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+  if (Iter == FunctionFlags.end() || Iter->first != Func) {
+    return createStringError("Shader Flags information of Function '" +
+                             Func->getName() + "' not found");
+  }


nit: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Suggested change

if (Iter == FunctionFlags.end() || Iter->first != Func) {

return createStringError("Shader Flags information of Function '" +

Func->getName() + "' not found");

}

if (Iter == FunctionFlags.end() || Iter->first != Func)

return createStringError("Shader Flags information of Function '" +

Func->getName() + "' not found");

The only way to have this fail is if we've invalidated the analysis (and failed to tell the pass manager) or we're trying to use it wrong. This should just be an assert.

nit: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Changed the if statement assert.

llvm-beanz · 2024-11-18T21:29:38Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

 }

 PreservedAnalyses ShaderFlagsAnalysisPrinter::run(Module &M,
                                                  ModuleAnalysisManager &AM) {
-  ComputedShaderFlags Flags = AM.getResult<ShaderFlagsAnalysis>(M);
-  Flags.print(OS);
+  DXILModuleShaderFlagsInfo FlagsInfo = AM.getResult<ShaderFlagsAnalysis>(M);


You could store the const & to avoid the copy here.

llvm-beanz · 2024-11-18T21:35:41Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+    if (Error E = SFMask.takeError()) {
+      M.getContext().diagnose(
+          DiagnosticInfoShaderFlags(M, toString(std::move(E))));
+    }


This should be done through a call to llvm::handleAllErrors. Something like:

Suggested change

if (Error E = SFMask.takeError()) {

M.getContext().diagnose(

DiagnosticInfoShaderFlags(M, toString(std::move(E))));

}

if (!SFMask)

return handleAllErrors(std::move(E),

[&](std::unique_ptr<ErrorInfoBase> EIB) -> Error {

M.getContext().diagnose(errorToDiagnosticInfo(EIB);

return Error::success();

});

This handles arrays of errors so that your function can return more than one error.

This should be done through a call to llvm::handleAllErrors. Something like:

This handles arrays of errors so that your function can return more than one error.

Deleted this error-handling code as a result of the assertion added in getShaderFlagsmask().

llvm-beanz · 2024-11-18T21:37:31Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+  for (const auto &F : M.getFunctionList()) {
+    if (F.isDeclaration())
+      continue;
+    ComputedShaderFlags CSF{};


nit: ComputedShaderFlags has a default constructor to zero itself out, the empty initializer list is unnecessary.

Suggested change

ComputedShaderFlags CSF{};

ComputedShaderFlags CSF;

nit: ComputedShaderFlags has a default constructor to zero itself out, the empty initializer list is unnecessary.

Changed.

llvm-beanz · 2024-11-18T21:38:59Z

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

+    if (Error E = EntrySFMask.takeError()) {
+      M.getContext().diagnose(
+          DiagnosticInfoTranslateMD(M, toString(std::move(E))));
+    }


This should also be a handleAllErrors call.

This should also be a handleAllErrors call.

Deleted this error-handling code as a result of the assertion added in getShaderFlagsmask().

llvm-beanz

Something I'm also just noticing. It doesn't look like any of the tests here actually trigger the errors. Should these even be errors? Should they instead be asserts?

bogner

This is looking pretty close. As both Damyan and Chris pointed out, this isn't the appropriate kind of error handling here - the only way to hit the error is to break an invariant, which means we don't need user facing errors at all. A few other comments below.

bogner · 2024-11-22T08:10:31Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+  std::pair<Function const *, ComputedShaderFlags> V{Func, {}};
+  const auto Iter = llvm::lower_bound(FunctionFlags, V);


Probably simpler to use a comparator function that just compares against the pair's first rather than constructing a sentinel to compare against.

Also llvm style prefers const Function * to Function const *

Changed to use comparator.

bogner · 2024-11-22T08:13:16Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+  if (Iter == FunctionFlags.end() || Iter->first != Func) {
+    return createStringError("Shader Flags information of Function '" +
+                             Func->getName() + "' not found");
+  }


The only way to have this fail is if we've invalidated the analysis (and failed to tell the pass manager) or we're trying to use it wrong. This should just be an assert.

bogner · 2024-11-22T08:23:58Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

+  Expected<const ComputedShaderFlags &>
+  getShaderFlagsMask(const Function *) const;
+  const ComputedShaderFlags getCombinedFlags() const;


A few things here:

getShaderFlagsMask should just return a const ComputedShaderFlags &, not an expected. The only way for this to fail if the analysis is in a valid state is if we call this with a function that isn't in the module somehow.

getCombinedFlags should also return a reference - no need for a copy here.

These names are inconsistent (FlagsMask vs Flags) even though they return the same type of thing. Maybe we should simplify a little and call them getFunctionFlags and getCombinedFlags

Changes made per suggestions.

bogner · 2024-11-22T08:24:51Z

llvm/lib/Target/DirectX/DXILShaderFlags.h

+  /// Vector of Function-Shader Flag mask pairs representing properties of each
+  /// of the functions in the module. Shader Flags of each function represent
+  /// both module-level and function-level flags


Worth noting that this is a sorted vector.

Worth noting that this is a sorted vector.

Updated comment accordingly.

bogner · 2024-11-22T08:26:26Z

llvm/lib/Target/DirectX/DXILShaderFlags.cpp

+void DXILModuleShaderFlagsInfo::updateFunctionFlags(ComputedShaderFlags &CSF,
+                                                    const Instruction &I) {


Why is this a member function of DXILModuleShaderFlagsInfo and not just a static function? It doesn't use or change any state from the object.

Why is this a member function of DXILModuleShaderFlagsInfo and not just a static function? It doesn't use or change any state from the object.

Changed it to a static function.

bogner · 2024-11-22T08:32:29Z

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions.ll

+; CHECK-NEXT: ; Note: shader requires additional functionality:
+; CHECK-NEXT: ;       Double-precision floating point
+; CHECK-NEXT: ;       Double-precision extensions for 11.1
+; CHECK-NEXT: ; Note: extra DXIL module flags:


It seems a bit ridiculous to print these notes after every function. Can't that be printed only for the module flags?

It seems a bit ridiculous to print these notes after every function. Can't that be printed only for the module flags?

Changes made.

Delete DiagnosticInfoShaderFlags and flag failure to find shader flags mask of a function as fatal error as it is not expected to be the case for well-formed modules. Restore ModuleFlags::initialize inplace of constructor Rename DXILModuleShaderFlagsInfo as ModuleShaderFlags Use getModuleFlags() in operator uint_64()

Print Module flags notes only once. Delete extraneous checks in test Declare updateFunctionFlags as static Use comparator instead of constructing a fuction-mask pair to search in getFunctionFlags()

bogner · 2024-11-22T21:40:52Z

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions-obj-test.ll

+; CHECK-NEXT:       Doubles:         true
+; CHECK-NOT:   {{[A-Za-z]+: +true}}
+; CHECK:            DX11_1_DoubleExtensions:         true


Suggested change

; CHECK-NEXT: Doubles: true

; CHECK-NOT: {{[A-Za-z]+: +true}}

; CHECK: DX11_1_DoubleExtensions: true

; CHECK: Doubles: true

; CHECK: DX11_1_DoubleExtensions: true

bogner · 2024-11-22T21:41:34Z

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

@@ -25,6 +25,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/Support/Error.h"


Unused include

Unused include

Deleted.

bogner · 2024-11-22T21:44:02Z

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions.ll

+; CHECK-NEXT: ; Shader Flags for Module Functions
+; CHECK-NEXT: ; Function test_fdiv_double : 0x00000044
+; CHECK-NEXT: ;
+; CHECK-NEXT: ; Function test_uitofp_i64 : 0x00000044
+; CHECK-NEXT: ;
+; CHECK-NEXT: ; Function test_sitofp_i64 : 0x00000044
+; CHECK-NEXT: ;
+; CHECK-NEXT: ; Function test_fptoui_i32 : 0x00000044
+; CHECK-NEXT: ;
+; CHECK-NEXT: ; Function test_fptosi_i64 : 0x00000044


Might be easier to read/update this test if the checks for each function were by the function definition. I also think we can probably dispense of the CHECK-NEXT in that case and not bother checking the empty lines - if the checks are all by the function definitions it's obvious if we missed one.

Might be easier to read/update this test if the checks for each function were by the function definition. I also think we can probably dispense of the CHECK-NEXT in that case and not bother checking the empty lines - if the checks are all by the function definitions it's obvious if we missed one.

Changes made.

bogner · 2024-11-22T23:05:41Z

llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions-obj-test.ll

+; CHECK:       Doubles:         true
 ; CHECK:            DX11_1_DoubleExtensions:         true


It'd be nice to have consistent whitespace across the check lines

llvmbot added the backend:DirectX label Oct 18, 2024

bharadwajy added 2 commits October 21, 2024 12:32

clang-format changes

397f70b

bharadwajy force-pushed the shader-flags/func-level-collection branch from cdfa0b5 to 397f70b Compare October 21, 2024 16:52

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

[DirectX] Extend existing Module pass to collect shader flags per-function #111867

Closed

damyanp requested changes Oct 22, 2024

View reviewed changes

bharadwajy changed the title ~~[NFC][DirectX] Infrastructure to collect shader flags for each function~~ [DirectX] Infrastructure to collect shader flags for each function Oct 28, 2024

bharadwajy requested a review from damyanp October 28, 2024 15:36

damyanp requested changes Oct 28, 2024

View reviewed changes

llvm/lib/Target/DirectX/DXILShaderFlags.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/DirectX/DXILShaderFlags.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/DirectX/DXILShaderFlags.cpp Outdated Show resolved Hide resolved

Compare functions by their names instead of constructing a pseudo-sig…

c02053a

…nature Non-empty Function names are unique in LLVM IR. Update the expected test output accordingly

bharadwajy requested a review from damyanp October 28, 2024 20:49

damyanp requested changes Oct 28, 2024

View reviewed changes

Fix incorrect lookup that expects a sorted vector done in a possibly …

47ab4c5

…unsorted vector Other changes based on latest PR feedback

bharadwajy requested a review from damyanp October 29, 2024 02:10

bharadwajy added 2 commits October 29, 2024 11:10

Use getFunctionList() instead of iterating Module for functions.

fa8ec60

Change return type of getShaderFlagsMask() to Expected<T>

a4f1e51

Delete unused class DiagnosticInfoShaderFlags

bogner reviewed Oct 30, 2024

View reviewed changes

bharadwajy requested a review from bogner October 31, 2024 23:32

damyanp requested changes Nov 1, 2024

View reviewed changes

Address PR feedback

31b0770

bharadwajy requested a review from damyanp November 4, 2024 18:12

damyanp reviewed Nov 6, 2024

View reviewed changes

bharadwajy requested a review from damyanp November 6, 2024 19:51

Delete unused function

56af02a

damyanp reviewed Nov 15, 2024

View reviewed changes

bharadwajy requested a review from damyanp November 15, 2024 21:54

damyanp reviewed Nov 15, 2024

View reviewed changes

damyanp self-requested a review November 15, 2024 23:24

Rename operator |= as merge

c6b3390

bharadwajy force-pushed the shader-flags/func-level-collection branch from be34fb3 to c6b3390 Compare November 18, 2024 13:59

llvm-beanz reviewed Nov 18, 2024

View reviewed changes

llvm-beanz reviewed Nov 19, 2024

View reviewed changes

bogner reviewed Nov 22, 2024

View reviewed changes

bharadwajy added 2 commits November 22, 2024 11:24

Rename getShaderFlagsMask() as getFunctionFlags()

07734f7

Print Module flags notes only once. Delete extraneous checks in test Declare updateFunctionFlags as static Use comparator instead of constructing a fuction-mask pair to search in getFunctionFlags()

bharadwajy requested review from bogner and llvm-beanz November 22, 2024 21:28

bogner reviewed Nov 22, 2024

View reviewed changes

Cleanup of CHECK directives in tests.

a0d2a31

bharadwajy requested a review from bogner November 22, 2024 22:36

bogner approved these changes Nov 22, 2024

View reviewed changes

Delete extraneous space in CHECK line

2cee00a

bharadwajy merged commit 96547de into llvm:main Nov 25, 2024
8 checks passed

-    if (Error E = SFMask.takeError()) {
-      M.getContext().diagnose(
-          DiagnosticInfoShaderFlags(M, toString(std::move(E))));
-    }
+   if (!SFMask)
+      return handleAllErrors(std::move(E),
+                    [&](std::unique_ptr<ErrorInfoBase> EIB) -> Error {
+                      M.getContext().diagnose(errorToDiagnosticInfo(EIB);
+                      return Error::success();
+                    });

		std::pair<Function const *, ComputedShaderFlags> V{Func, {}};
		const auto Iter = llvm::lower_bound(FunctionFlags, V);

		void DXILModuleShaderFlagsInfo::updateFunctionFlags(ComputedShaderFlags &CSF,
		const Instruction &I) {

		; CHECK: Doubles: true
		; CHECK: DX11_1_DoubleExtensions: true

[DirectX] Infrastructure to collect shader flags for each function #112967

[DirectX] Infrastructure to collect shader flags for each function #112967

Conversation

bharadwajy commented Oct 18, 2024 • edited Loading

llvmbot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024 • edited Loading

bogner commented Oct 22, 2024

bharadwajy commented Oct 28, 2024

damyanp left a comment

Choose a reason for hiding this comment

bharadwajy commented Oct 28, 2024

damyanp commented Oct 28, 2024

bharadwajy commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damyanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damyanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvm-beanz left a comment

Choose a reason for hiding this comment

bogner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharadwajy commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading