Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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.getFunctionFlags()) {
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
112 changes: 97 additions & 15 deletions llvm/lib/Target/DirectX/DXILShaderFlags.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,78 @@

#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/Error.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) {
Type *Ty = I.getType();
if (Ty->isDoubleTy()) {
Flags.Doubles = true;
namespace {
/// A simple Wrapper DiagnosticInfo that generates Module-level diagnostic
/// for Shader Flags Analysis 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

void DXILModuleShaderFlagsInfo::updateFuctionFlags(ComputedShaderFlags &CSF,
const Instruction &I) {
if (!CSF.Doubles) {
CSF.Doubles = I.getType()->isDoubleTy();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Suggested change
if (!CSF.Doubles) {
CSF.Doubles = I.getType()->isDoubleTy();
}
if (!CSF.Doubles)
CSF.Doubles = I.getType()->isDoubleTy();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per LLVM coding style.

if (!CSF.Doubles) {
for (Value *Op : I.operands()) {
CSF.Doubles |= Op->getType()->isDoubleTy();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Suggested change
for (Value *Op : I.operands()) {
CSF.Doubles |= Op->getType()->isDoubleTy();
}
for (Value *Op : I.operands())
CSF.Doubles |= Op->getType()->isDoubleTy();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per LLVM coding style.

}
if (CSF.Doubles) {
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)
bool DXILModuleShaderFlagsInfo::initialize(const Module &M) {
// Collect shader flags for each of the functions
for (const auto &F : M.getFunctionList()) {
if (F.isDeclaration())
continue;
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;
updateFuctionFlags(CSF, I);
// Insert shader flag mask for function F
FunctionFlags.push_back({&F, CSF});
}
llvm::sort(FunctionFlags);
return true;
}

void ComputedShaderFlags::print(raw_ostream &OS) const {
Expand All @@ -63,17 +105,57 @@ void ComputedShaderFlags::print(raw_ostream &OS) const {
OS << ";\n";
}

const SmallVector<std::pair<Function const *, ComputedShaderFlags>> &
DXILModuleShaderFlagsInfo::getFunctionFlags() const {
return FunctionFlags;
}

const ComputedShaderFlags &DXILModuleShaderFlagsInfo::getModuleFlags() const {
return ModuleFlags;
}

Expected<const ComputedShaderFlags &>
DXILModuleShaderFlagsInfo::getShaderFlagsMask(const Function *Func) const {
std::pair<Function const *, ComputedShaderFlags> V{Func, {}};
const auto Iter = llvm::lower_bound(FunctionFlags, V);
Copy link
Contributor

Choose a reason for hiding this comment

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

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 *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use comparator.

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

Choose a reason for hiding this comment

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

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");

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return Iter->second;
}

AnalysisKey ShaderFlagsAnalysis::Key;

ComputedShaderFlags ShaderFlagsAnalysis::run(Module &M,
ModuleAnalysisManager &AM) {
return ComputedShaderFlags::computeFlags(M);
DXILModuleShaderFlagsInfo ShaderFlagsAnalysis::run(Module &M,
ModuleAnalysisManager &AM) {
DXILModuleShaderFlagsInfo MSFI;
MSFI.initialize(M);
return MSFI;
}

bool ShaderFlagsAnalysisWrapper::runOnModule(Module &M) {
MSFI.initialize(M);
return false;
}

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

Choose a reason for hiding this comment

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

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

OS << "; Shader Flags mask for Module:\n";
FlagsInfo.getModuleFlags().print(OS);
for (const auto &F : M.getFunctionList()) {
if (F.isDeclaration())
continue;
OS << "; Shader Flags mask for Function: " << F.getName() << "\n";
auto SFMask = FlagsInfo.getShaderFlagsMask(&F);
if (Error E = SFMask.takeError()) {
M.getContext().diagnose(
DiagnosticInfoShaderFlags(M, toString(std::move(E))));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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().

SFMask->print(OS);
}

return PreservedAnalyses::all();
}

Expand Down
43 changes: 34 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,48 @@ struct ComputedShaderFlags {
return FeatureFlags;
}

static ComputedShaderFlags computeFlags(Module &M);
uint64_t getModuleFlags() const {
uint64_t ModuleFlags = 0;
#define DXIL_MODULE_FLAG(DxilModuleBit, FlagName, Str) \
ModuleFlags |= FlagName ? getMask(DxilModuleBit) : 0ull;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

#include "llvm/BinaryFormat/DXContainerConstants.def"
return ModuleFlags;
}

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

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

bool initialize(const Module &M);
Expected<const ComputedShaderFlags &>
getShaderFlagsMask(const Function *Func) const;
bool hasShaderFlagsMask(const Function *Func) const;
const ComputedShaderFlags &getModuleFlags() const;
const SmallVector<std::pair<Function const *, ComputedShaderFlags>> &
getFunctionFlags() const;

private:
// Shader Flag mask representing module-level properties. These are
// represented using the macro DXIL_MODULE_FLAG
ComputedShaderFlags ModuleFlags;
// Vector of Function-Shader Flag mask pairs representing properties of each
// of the functions in the module. Shader Flags of each function are those
// represented using the macro SHADER_FEATURE_FLAG.
SmallVector<std::pair<Function const *, ComputedShaderFlags>> FunctionFlags;
void updateFuctionFlags(ComputedShaderFlags &CSF, const Instruction &I);
};

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 +120,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
48 changes: 30 additions & 18 deletions llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "llvm/IR/Module.h"
#include "llvm/InitializePasses.h"
#include "llvm/Pass.h"
#include "llvm/Support/Error.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused include

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused include

Deleted.

#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/VersionTuple.h"
#include "llvm/TargetParser/Triple.h"
Expand Down Expand Up @@ -286,11 +287,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 +298,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 +314,38 @@ 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.getModuleFlags();
for (const auto &FunFlags : ShaderFlags.getFunctionFlags()) {
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;
Expected<const ComputedShaderFlags &> ECSF =
ShaderFlags.getShaderFlagsMask(EntryProp.Entry);
if (Error E = ECSF.takeError()) {
M.getContext().diagnose(
DiagnosticInfoTranslateMD(M, toString(std::move(E))));
}

// 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.getModuleFlags();
}
if (MMDI.ShaderProfile != Triple::EnvironmentType::Library) {
if (EntryProp.ShaderStage != MMDI.ShaderProfile) {
M.getContext().diagnose(DiagnosticInfoTranslateMD(
Expand Down Expand Up @@ -361,7 +373,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 +405,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

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"}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

; CHECK-NOT: {{[A-Za-z]+: +true}}
; CHECK: NextUnusedBit: false
; CHECK: ...
Loading