Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/lib/Target/DirectX/DXContainerGlobals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ GlobalVariable *DXContainerGlobals::getFeatureFlags(Module &M) {
// of the shader flags of all functions in the module. Need to verify
// and modify the computation of feature flags to be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an issue tracking this?

uint64_t ConsolidatedFeatureFlags = 0;
for (const auto &FuncFlags : MSFI.FuncShaderFlagsMap) {
for (const auto &FuncFlags : MSFI.FuncShaderFlagsVec) {
ConsolidatedFeatureFlags |= FuncFlags.second.getFeatureFlags();
}

Expand Down
130 changes: 114 additions & 16 deletions llvm/lib/Target/DirectX/DXILShaderFlags.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,43 +13,114 @@

#include "DXILShaderFlags.h"
#include "DirectX.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/DiagnosticPrinter.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Module.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/raw_ostream.h"

using namespace llvm;
using namespace llvm::dxil;

static void updateFlags(DXILModuleShaderFlagsInfo &MSFI, const Instruction &I) {
ComputedShaderFlags &FSF = MSFI.FuncShaderFlagsMap[I.getFunction()];
namespace {
/// A simple Wrapper DiagnosticInfo that generates Module-level diagnostic
/// for ShaderFlagsAnalysis pass
class DiagnosticInfoShaderFlags : public DiagnosticInfo {
private:
const Twine &Msg;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const Module &Mod;

public:
/// \p M is the module for which the diagnostic is being emitted. \p Msg is
/// the message to show. Note that this class does not copy this message, so
/// this reference must be valid for the whole life time of the diagnostic.
DiagnosticInfoShaderFlags(const Module &M, const Twine &Msg,
DiagnosticSeverity Severity = DS_Error)
: DiagnosticInfo(DK_Unsupported, Severity), Msg(Msg), Mod(M) {}

void print(DiagnosticPrinter &DP) const override {
DP << Mod.getName() << ": " << Msg << '\n';
}
};
} // namespace

static void updateFlags(ComputedShaderFlags &CSF, const Instruction &I) {
Type *Ty = I.getType();
if (Ty->isDoubleTy()) {
FSF.Doubles = true;
bool DoubleTyInUse = Ty->isDoubleTy();
for (Value *Op : I.operands()) {
DoubleTyInUse |= Op->getType()->isDoubleTy();
}

if (DoubleTyInUse) {
CSF.Doubles = true;
switch (I.getOpcode()) {
case Instruction::FDiv:
case Instruction::UIToFP:
case Instruction::SIToFP:
case Instruction::FPToUI:
case Instruction::FPToSI:
FSF.DX11_1_DoubleExtensions = true;
// TODO: To be set if I is a call to DXIL intrinsic DXIL::Opcode::Fma
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have an issue for this?

#114554.

CSF.DX11_1_DoubleExtensions = true;
break;
}
}
}

static bool compareFuncSFPairs(const FuncShaderFlagsMask &First,
const FuncShaderFlagsMask &Second) {
// Construct string representation of the functions in each pair
// as "retTypefunctionNamearg1Typearg2Ty..." where the function signature is
// retType functionName(arg1Type, arg2Ty,...). Spaces, braces and commas are
// omitted in the string representation of the signature. This allows
// determining a consistent lexicographical order of all functions by their
// signatures.
std::string FirstFunSig;
std::string SecondFunSig;
raw_string_ostream FRSO(FirstFunSig);
raw_string_ostream SRSO(SecondFunSig);

// Return type
First.first->getReturnType()->print(FRSO);
Second.first->getReturnType()->print(SRSO);
// Function name
FRSO << First.first->getName();
SRSO << Second.first->getName();
// Argument types
for (const Argument &Arg : First.first->args()) {
Arg.getType()->print(FRSO);
}
for (const Argument &Arg : Second.first->args()) {
Arg.getType()->print(SRSO);
}
FRSO.flush();
SRSO.flush();

return FRSO.str().compare(SRSO.str()) < 0;
}

static DXILModuleShaderFlagsInfo computeFlags(Module &M) {
DXILModuleShaderFlagsInfo MSFI;
for (const auto &F : M) {
for (auto &F : M) {
if (F.isDeclaration())
continue;
if (!MSFI.FuncShaderFlagsMap.contains(&F)) {
ComputedShaderFlags CSF{};
MSFI.FuncShaderFlagsMap[&F] = CSF;
// Each of the functions in a module are unique. Hence no prior shader flags
// mask of the function should be present.
if (MSFI.hasShaderFlagsMask(&F)) {
M.getContext().diagnose(DiagnosticInfoShaderFlags(
M, "Shader Flags mask for Function '" + Twine(F.getName()) +
"' already exits"));
}
ComputedShaderFlags CSF{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Suggested change
ComputedShaderFlags CSF{};
ComputedShaderFlags CSF;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Changed.

for (const auto &BB : F)
for (const auto &I : BB)
updateFlags(MSFI, I);
updateFlags(CSF, I);
// Insert shader flag mask for function F
MSFI.FuncShaderFlagsVec.push_back({&F, CSF});
}
// Sort MSFI.FuncShaderFlagsVec for later lookup that uses binary search
llvm::sort(MSFI.FuncShaderFlagsVec, compareFuncSFPairs);
return MSFI;
}

Expand All @@ -71,6 +142,38 @@ void ComputedShaderFlags::print(raw_ostream &OS) const {
OS << ";\n";
}

void DXILModuleShaderFlagsInfo::print(raw_ostream &OS) const {
OS << "; Shader Flags mask for Module:\n";
ModuleFlags.print(OS);
for (auto SF : FuncShaderFlagsVec) {
OS << "; Shader Flags mask for Function: " << SF.first->getName() << "\n";
SF.second.print(OS);
}
}

const ComputedShaderFlags
DXILModuleShaderFlagsInfo::getShaderFlagsMask(const Function *Func) const {
FuncShaderFlagsMask V{Func, {}};
auto Iter = llvm::lower_bound(FuncShaderFlagsVec, V, compareFuncSFPairs);
if (Iter == FuncShaderFlagsVec.end()) {
Func->getContext().diagnose(DiagnosticInfoShaderFlags(
*(Func->getParent()), "Shader Flags information of Function '" +
Twine(Func->getName()) + "' not found"));
}
if (Iter->first != Func) {
Func->getContext().diagnose(DiagnosticInfoShaderFlags(
*(Func->getParent()),
"Inconsistent Shader Flags information of Function '" +
Twine(Func->getName()) + "' retrieved"));
}
return Iter->second;
}

bool DXILModuleShaderFlagsInfo::hasShaderFlagsMask(const Function *Func) const {
FuncShaderFlagsMask V{Func, {}};
return llvm::binary_search(FuncShaderFlagsVec, V);
}

AnalysisKey ShaderFlagsAnalysis::Key;

DXILModuleShaderFlagsInfo ShaderFlagsAnalysis::run(Module &M,
Expand All @@ -86,12 +189,7 @@ bool ShaderFlagsAnalysisWrapper::runOnModule(Module &M) {
PreservedAnalyses ShaderFlagsAnalysisPrinter::run(Module &M,
ModuleAnalysisManager &AM) {
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);
}
Flags.print(OS);
return PreservedAnalyses::all();
}

Expand Down
15 changes: 9 additions & 6 deletions llvm/lib/Target/DirectX/DXILShaderFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#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"
Expand Down Expand Up @@ -66,14 +65,18 @@ struct ComputedShaderFlags {
LLVM_DUMP_METHOD void dump() const { print(); }
};

using FunctionShaderFlagsMap =
SmallDenseMap<Function const *, ComputedShaderFlags>;
using FuncShaderFlagsMask = std::pair<Function const *, ComputedShaderFlags>;
using FunctionShaderFlagsVec = SmallVector<FuncShaderFlagsMask>;
struct DXILModuleShaderFlagsInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// 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;
// Vector of Function-Shader Flag mask pairs representing properties of each
// of the functions in the module
FunctionShaderFlagsVec FuncShaderFlagsVec;

const ComputedShaderFlags getShaderFlagsMask(const Function *Func) const;
bool hasShaderFlagsMask(const Function *Func) const;
void print(raw_ostream &OS = dbgs()) const;
};

class ShaderFlagsAnalysis : public AnalysisInfoMixin<ShaderFlagsAnalysis> {
Expand Down
13 changes: 4 additions & 9 deletions llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ static void translateMetadata(Module &M, const DXILResourceMap &DRM,
// 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) {
for (const auto &FunFlags : ShaderFlags.FuncShaderFlagsVec) {
ConsolidatedMask |= FunFlags.second;
}
EntryFnMDNodes.emplace_back(
Expand All @@ -329,20 +329,15 @@ static void translateMetadata(Module &M, const DXILResourceMap &DRM,
}

for (const EntryProperties &EntryProp : MMDI.EntryPropertyVec) {
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"));
}
ComputedShaderFlags ECSF = ShaderFlags.getShaderFlagsMask(EntryProp.Entry);
// 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;
// EntryProp.Entry has no call instructions.
EntryShaderFlags = ECSF | ShaderFlags.ModuleFlags;
}
if (MMDI.ShaderProfile != Triple::EnvironmentType::Library) {
if (EntryProp.ShaderStage != MMDI.ShaderProfile) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC

target triple = "dxil-pc-shadermodel6.7-library"
define double @div(double %a, double %b) #0 {
%res = fdiv double %a, %b
ret double %res
}

attributes #0 = { convergent norecurse nounwind "hlsl.export"}

; DXC: - Name: SFI0
; DXC-NEXT: Size: 8
; DXC-NEXT: Flags:
; DXC-NEXT: Doubles: true
; DXC-NOT: {{[A-Za-z]+: +true}}
; DXC: DX11_1_DoubleExtensions: true
; DXC-NOT: {{[A-Za-z]+: +true}}
; DXC: NextUnusedBit: false
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

; DXC: ...
77 changes: 62 additions & 15 deletions llvm/test/CodeGen/DirectX/ShaderFlags/double-extensions.ll
Original file line number Diff line number Diff line change
@@ -1,27 +1,74 @@
; RUN: opt -S --passes="print-dx-shader-flags" 2>&1 %s | FileCheck %s
; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC

target triple = "dxil-pc-shadermodel6.7-library"

; CHECK: ; Shader Flags Value: 0x00000044
; CHECK: ; Note: shader requires additional functionality:
; CHECK: ; Shader Flags mask for Module:
; CHECK-NEXT: ; Shader Flags Value: 0x00000000
; CHECK-NEXT: ;
; CHECK-NEXT: ; Shader Flags mask for Function: test_fdiv_double
; CHECK-NEXT: ; Shader Flags Value: 0x00000044
; CHECK-NEXT: ;
; 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:
; CHECK-NEXT: {{^;$}}
define double @div(double %a, double %b) #0 {
; CHECK-NEXT: ;
; CHECK-NEXT: ; Shader Flags mask for Function: test_sitofp_i64
; CHECK-NEXT: ; Shader Flags Value: 0x00000044
; CHECK-NEXT: ;
; 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Changes made.

; CHECK-NEXT: ;
; CHECK-NEXT: ; Shader Flags mask for Function: test_uitofp_i64
; CHECK-NEXT: ; Shader Flags Value: 0x00000044
; CHECK-NEXT: ;
; 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:
; CHECK-NEXT: ;
; CHECK-NEXT: ; Shader Flags mask for Function: test_fptoui_i32
; CHECK-NEXT: ; Shader Flags Value: 0x00000044
; CHECK-NEXT: ;
; 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:
; CHECK-NEXT: ;
; CHECK-NEXT: ; Shader Flags mask for Function: test_fptosi_i64
; CHECK-NEXT: ; Shader Flags Value: 0x00000044
; CHECK-NEXT: ;
; 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:
; CHECK-NEXT: ;

define double @test_fdiv_double(double %a, double %b) #0 {
%res = fdiv double %a, %b
ret double %res
}

attributes #0 = { convergent norecurse nounwind "hlsl.export"}
define double @test_uitofp_i64(i64 %a) #0 {
%r = uitofp i64 %a to double
ret double %r
}

define double @test_sitofp_i64(i64 %a) #0 {
%r = sitofp i64 %a to double
ret double %r
}

; DXC: - Name: SFI0
; DXC-NEXT: Size: 8
; DXC-NEXT: Flags:
; DXC-NEXT: Doubles: true
; DXC-NOT: {{[A-Za-z]+: +true}}
; DXC: DX11_1_DoubleExtensions: true
; DXC-NOT: {{[A-Za-z]+: +true}}
; DXC: NextUnusedBit: false
; DXC: ...
define i32 @test_fptoui_i32(double %a) #0 {
%r = fptoui double %a to i32
ret i32 %r
}

define i64 @test_fptosi_i64(double %a) #0 {
%r = fptosi double %a to i64
ret i64 %r
}

attributes #0 = { convergent norecurse nounwind "hlsl.export"}
5 changes: 4 additions & 1 deletion llvm/test/CodeGen/DirectX/ShaderFlags/doubles.ll
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

target triple = "dxil-pc-shadermodel6.7-library"

; CHECK: ; Shader Flags Value: 0x00000004
; CHECK: ; Shader Flags mask for Module:
; CHECK-NEXT: ; Shader Flags Value: 0x00000000
; CHECK: ; Shader Flags mask for Function: add
; CHECK-NEXT: ; Shader Flags Value: 0x00000004
; CHECK: ; Note: shader requires additional functionality:
; CHECK-NEXT: ; Double-precision floating point
; CHECK-NEXT: ; Note: extra DXIL module flags:
Expand Down
6 changes: 5 additions & 1 deletion llvm/test/CodeGen/DirectX/ShaderFlags/no_flags.ll
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

target triple = "dxil-pc-shadermodel6.7-library"

; CHECK: ; Shader Flags Value: 0x00000000
; CHECK: ; Shader Flags mask for Module:
; CHECK-NEXT: ; Shader Flags Value: 0x00000000
;
; CHECK: ; Shader Flags mask for Function: add
; CHECK-NEXT: ; Shader Flags Value: 0x00000000
define i32 @add(i32 %a, i32 %b) {
%sum = add i32 %a, %b
ret i32 %sum
Expand Down
Loading