Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
15 changes: 10 additions & 5 deletions llvm/lib/Target/DirectX/DXContainerGlobals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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.FuncShaderFlagsVec) {
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");
}

Expand Down
142 changes: 129 additions & 13 deletions llvm/lib/Target/DirectX/DXILShaderFlags.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,115 @@

#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(ComputedShaderFlags &Flags, const Instruction &I) {
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()) {
Flags.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:
Flags.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;
}
}
}

ComputedShaderFlags ComputedShaderFlags::computeFlags(Module &M) {
ComputedShaderFlags Flags;
for (const auto &F : M)
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 (auto &F : M) {
if (F.isDeclaration())
continue;
// 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(Flags, I);
return Flags;
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;
}

void ComputedShaderFlags::print(raw_ostream &OS) const {
Expand All @@ -63,16 +142,53 @@ 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;

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);
DXILModuleShaderFlagsInfo Flags = AM.getResult<ShaderFlagsAnalysis>(M);
Flags.print(OS);
return PreservedAnalyses::all();
}
Expand Down
29 changes: 20 additions & 9 deletions llvm/lib/Target/DirectX/DXILShaderFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#ifndef LLVM_TARGET_DIRECTX_DXILSHADERFLAGS_H
#define LLVM_TARGET_DIRECTX_DXILSHADERFLAGS_H

#include "llvm/IR/Function.h"
#include "llvm/IR/PassManager.h"
#include "llvm/Pass.h"
#include "llvm/Support/Compiler.h"
Expand Down Expand Up @@ -60,21 +61,34 @@ struct ComputedShaderFlags {
return FeatureFlags;
}

static ComputedShaderFlags computeFlags(Module &M);
void print(raw_ostream &OS = dbgs()) const;
LLVM_DUMP_METHOD void dump() const { print(); }
};

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;
// 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> {
friend AnalysisInfoMixin<ShaderFlagsAnalysis>;
static AnalysisKey Key;

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.
Expand All @@ -92,19 +106,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();
Expand Down
41 changes: 23 additions & 18 deletions llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -318,22 +313,32 @@ 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.FuncShaderFlagsVec) {
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;
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
// EntryProp.Entry has no call instructions.
EntryShaderFlags = ECSF | ShaderFlags.ModuleFlags;
}
if (MMDI.ShaderProfile != Triple::EnvironmentType::Library) {
if (EntryProp.ShaderStage != MMDI.ShaderProfile) {
M.getContext().diagnose(DiagnosticInfoTranslateMD(
Expand Down Expand Up @@ -361,7 +366,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);

Expand Down Expand Up @@ -393,7 +398,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();
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: ...
Loading
Loading