Skip to content

Allow native vectors for LLVM operations #7155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pow2clk merged 18 commits into from
Mar 26, 2025
Merged

Allow native vectors for LLVM operations #7155

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
68a284b
Allow native vectors for LLVM operations
Dec 3, 2024
556f6e6
fix assert for tesselation patch template args
Feb 18, 2025
654bf8a
Merge 'origin/main' into longvec_operators
Mar 20, 2025
248fe80
Scalarization pass enable improvements
Mar 12, 2025
f1de617
clang-format
Mar 20, 2025
47d42f0
simplify bitcast module retrieval
Mar 20, 2025
d480caa
Add long vec scalarizer test
Mar 21, 2025
c2ee61b
Clean up test mistakes that failed on other platforms
Mar 21, 2025
a3a39b8
another constant case I caught just in case
Mar 21, 2025
6f7f9ec
Respond to feedback
Mar 24, 2025
1596d99
clang-format
Mar 24, 2025
411b921
fixup mistakenly submitted validation code
Mar 24, 2025
8dc2a3e
clang-format
Mar 24, 2025
46829dd
Respond to feedback
Mar 25, 2025
9002bee
Allow loading DXIL shader model without module
Mar 25, 2025
ba831c0
small clang-format change
Mar 25, 2025
7292edb
Retrieve dxil version instead of shader model from metadata
Mar 26, 2025
96ac2e2
trim down IR passes
Mar 26, 2025
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
36 changes: 36 additions & 0 deletions include/dxc/DXIL/DxilInstructions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,42 @@ struct LlvmInst_VAArg {
bool isAllowed() const { return false; }
};

/// This instruction extracts from vector
struct LlvmInst_ExtractElement {
llvm::Instruction *Instr;
// Construction and identification
LlvmInst_ExtractElement(llvm::Instruction *pInstr) : Instr(pInstr) {}
operator bool() const {
return Instr->getOpcode() == llvm::Instruction::ExtractElement;
}
// Validation support
bool isAllowed() const { return true; }
Copy link
Copy Markdown
Collaborator

@farzonl farzonl Mar 21, 2025

Choose a reason for hiding this comment

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

A little suprised to see ExtractElement`InsertElement\ShuffleVector` weren't already defined since I expected at least the first two as required for scalarization which DXIL already supports.

If these are new for vectorization, should isAllowed() be checking IsSM69Plus() instead of always returning true?

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 24, 2025

Choose a reason for hiding this comment

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

Scalarization took place before DXIL was generated. The output has no native vectors in non-lib shaders and so had no need for these functions. Intermediate steps might have these, but were disallowed for validation of final output. For similar reasons as their inclusion in intermediate steps, library shaders would allow these ops as they preserved native vectors in function interfaces. The check for library shader allowed ops accounts for this in validation.

DirectXShaderCompiler/lib/DxilValidation/DxilValidation.cpp

Line 2652 in b646ad3

case Instruction::InsertElement:

The inclusion here adds them to the list of instructions that are approved for general validation by IsLLVMInstructionAllowed, a generated function that merely
returns whether the opcode is in the list of approved LLVM instructions for a non-lib. It is generated into DxilValidationImpl.inc and called here:

DirectXShaderCompiler/lib/DxilValidation/DxilValidation.cpp

Line 2694 in b646ad3

if (!IsLLVMInstructionAllowed(I)) {

Similar to this function, this member is meant to return just true or false if it is allowed at all. I should add a 6.9 check to validation though.

};

/// This instruction inserts into vector
struct LlvmInst_InsertElement {
llvm::Instruction *Instr;
// Construction and identification
LlvmInst_InsertElement(llvm::Instruction *pInstr) : Instr(pInstr) {}
operator bool() const {
return Instr->getOpcode() == llvm::Instruction::InsertElement;
}
// Validation support
bool isAllowed() const { return true; }
};

/// This instruction Shuffle two vectors
struct LlvmInst_ShuffleVector {
llvm::Instruction *Instr;
// Construction and identification
LlvmInst_ShuffleVector(llvm::Instruction *pInstr) : Instr(pInstr) {}
operator bool() const {
return Instr->getOpcode() == llvm::Instruction::ShuffleVector;
}
// Validation support
bool isAllowed() const { return true; }
};

/// This instruction extracts from aggregate
struct LlvmInst_ExtractValue {
llvm::Instruction *Instr;
Expand Down
23 changes: 22 additions & 1 deletion lib/DxilValidation/DxilValidation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2193,6 +2193,9 @@ static bool ValidateType(Type *Ty, ValidationContext &ValCtx,
return true;

if (Ty->isVectorTy()) {
if (Ty->getVectorNumElements() > 1 &&
ValCtx.DxilMod.GetShaderModel()->IsSM69Plus())
return true;
ValCtx.EmitTypeError(Ty, ValidationRule::TypesNoVector);
return false;
}
Expand Down Expand Up @@ -2669,6 +2672,23 @@ static bool IsLLVMInstructionAllowedForLib(Instruction &I,
}
}

// Shader model specific checks for valid LLVM instructions.
// Currently only checks for pre 6.9 usage of vector operations.
// Returns false if shader model is pre 6.9 and I represents a vector
// operation. Returns true otherwise.
static bool IsLLVMInstructionAllowedForShaderModel(Instruction &I,
ValidationContext &ValCtx) {
if (ValCtx.DxilMod.GetShaderModel()->IsSM69Plus())
return true;
unsigned OpCode = I.getOpcode();
if (OpCode == Instruction::InsertElement ||
OpCode == Instruction::ExtractElement ||
OpCode == Instruction::ShuffleVector)
return false;

return true;
}

static void ValidateFunctionBody(Function *F, ValidationContext &ValCtx) {
bool SupportsMinPrecision =
ValCtx.DxilMod.GetGlobalFlags() & DXIL::kEnableMinPrecision;
Expand All @@ -2691,7 +2711,8 @@ static void ValidateFunctionBody(Function *F, ValidationContext &ValCtx) {
}

// Instructions must be allowed.
if (!IsLLVMInstructionAllowed(I)) {
if (!IsLLVMInstructionAllowed(I) ||
!IsLLVMInstructionAllowedForShaderModel(I, ValCtx)) {
if (!IsLLVMInstructionAllowedForLib(I, ValCtx)) {
ValCtx.EmitInstrError(&I, ValidationRule::InstrAllowed);
continue;
Expand Down
6 changes: 6 additions & 0 deletions lib/HLSL/DxilLinker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,12 @@ void DxilLinkJob::RunPreparePass(Module &M) {
// For static global handle.
PM.add(createLowerStaticGlobalIntoAlloca());

// Change dynamic indexing vector to array where vectors aren't
// supported, but might be there from the initial compile.
if (!pSM->IsSM69Plus())
PM.add(
createDynamicIndexingVectorToArrayPass(false /* ReplaceAllVector */));

// Remove MultiDimArray from function call arg.
PM.add(createMultiDimArrayToOneDimArrayPass());

Expand Down
71 changes: 47 additions & 24 deletions lib/HLSL/HLMatrixBitcastLowerPass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,17 @@ Type *TryLowerMatTy(Type *Ty) {
}

class MatrixBitcastLowerPass : public FunctionPass {
bool SupportsVectors = false;

public:
static char ID; // Pass identification, replacement for typeid
explicit MatrixBitcastLowerPass() : FunctionPass(ID) {}

StringRef getPassName() const override { return "Matrix Bitcast lower"; }
bool runOnFunction(Function &F) override {
DxilModule &DM = F.getParent()->GetOrCreateDxilModule();
SupportsVectors = DM.GetShaderModel()->IsSM69Plus();
Copy link
Copy Markdown
Contributor

@tex3d tex3d Mar 25, 2025

Choose a reason for hiding this comment

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

If you want to avoid full DxilModule dependency, I think it would be reasonable to have something that can look up the DXIL version in the module without loading full metadata, whether or not it's HLM or DM.

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 25, 2025

Choose a reason for hiding this comment

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

I like this idea. I know I mentioned it somewhere before. I'd prefer it not block this change though.


bool bUpdated = false;
std::unordered_set<BitCastInst *> matCastSet;
for (auto blkIt = F.begin(); blkIt != F.end(); ++blkIt) {
Expand All @@ -100,7 +104,6 @@ class MatrixBitcastLowerPass : public FunctionPass {
}
}

DxilModule &DM = F.getParent()->GetOrCreateDxilModule();
// Remove bitcast which has CallInst user.
if (DM.GetShaderModel()->IsLib()) {
for (auto it = matCastSet.begin(); it != matCastSet.end();) {
Expand All @@ -113,13 +116,13 @@ class MatrixBitcastLowerPass : public FunctionPass {

// Lower matrix first.
for (BitCastInst *BCI : matCastSet) {
lowerMatrix(BCI, BCI->getOperand(0));
lowerMatrix(DM, BCI, BCI->getOperand(0));
}
return bUpdated;
}

private:
void lowerMatrix(Instruction *M, Value *A);
void lowerMatrix(DxilModule &DM, Instruction *M, Value *A);
bool hasCallUser(Instruction *M);
};

Expand Down Expand Up @@ -180,44 +183,56 @@ Value *CreateEltGEP(Value *A, unsigned i, Value *zeroIdx,
}
} // namespace

void MatrixBitcastLowerPass::lowerMatrix(Instruction *M, Value *A) {
void MatrixBitcastLowerPass::lowerMatrix(DxilModule &DM, Instruction *M,
Copy link
Copy Markdown
Contributor

@tex3d tex3d Mar 25, 2025

Choose a reason for hiding this comment

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

I'm not seeing any use of DM, other than the recursive one required by this function signature.

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 25, 2025

Choose a reason for hiding this comment

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

Yeah, it's a relic from when I was getting the version from it.

Value *A) {
for (auto it = M->user_begin(); it != M->user_end();) {
User *U = *(it++);
if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(U)) {
Type *EltTy = GEP->getType()->getPointerElementType();
if (HLMatrixType::isa(EltTy)) {
if (HLMatrixType MatTy = HLMatrixType::dyn_cast(EltTy)) {
// Change gep matrixArray, 0, index
// into
// gep oneDimArray, 0, index * matSize
IRBuilder<> Builder(GEP);
SmallVector<Value *, 2> idxList(GEP->idx_begin(), GEP->idx_end());
DXASSERT(idxList.size() == 2,
"else not one dim matrix array index to matrix");

HLMatrixType MatTy = HLMatrixType::cast(EltTy);
Value *matSize = Builder.getInt32(MatTy.getNumElements());
idxList.back() = Builder.CreateMul(idxList.back(), matSize);
unsigned NumElts = MatTy.getNumElements();
if (!SupportsVectors || NumElts == 1) {
Value *MatSize = Builder.getInt32(NumElts);
idxList.back() = Builder.CreateMul(idxList.back(), MatSize);
}
Value *NewGEP = Builder.CreateGEP(A, idxList);
lowerMatrix(GEP, NewGEP);
lowerMatrix(DM, GEP, NewGEP);
DXASSERT(GEP->user_empty(), "else lower matrix fail");
GEP->eraseFromParent();
} else {
DXASSERT(0, "invalid GEP for matrix");
}
} else if (BitCastInst *BCI = dyn_cast<BitCastInst>(U)) {
lowerMatrix(BCI, A);
lowerMatrix(DM, BCI, A);
DXASSERT(BCI->user_empty(), "else lower matrix fail");
BCI->eraseFromParent();
} else if (LoadInst *LI = dyn_cast<LoadInst>(U)) {
if (VectorType *Ty = dyn_cast<VectorType>(LI->getType())) {
IRBuilder<> Builder(LI);
Value *zeroIdx = Builder.getInt32(0);
unsigned vecSize = Ty->getNumElements();
Value *NewVec = UndefValue::get(LI->getType());
for (unsigned i = 0; i < vecSize; i++) {
Value *GEP = CreateEltGEP(A, i, zeroIdx, Builder);
Value *Elt = Builder.CreateLoad(GEP);
NewVec = Builder.CreateInsertElement(NewVec, Elt, i);
Value *NewVec = nullptr;
unsigned VecSize = Ty->getVectorNumElements();
if (SupportsVectors && VecSize > 1) {
// Create a replacement load using the vector pointer.
Instruction *NewLd = LI->clone();
unsigned VecIdx = NewLd->getNumOperands() - 1;
NewLd->setOperand(VecIdx, A);
Builder.Insert(NewLd);
NewVec = NewLd;
} else {
Value *zeroIdx = Builder.getInt32(0);
NewVec = UndefValue::get(LI->getType());
for (unsigned i = 0; i < VecSize; i++) {
Value *GEP = CreateEltGEP(A, i, zeroIdx, Builder);
Value *Elt = Builder.CreateLoad(GEP);
NewVec = Builder.CreateInsertElement(NewVec, Elt, i);
}
Copy link
Copy Markdown
Contributor

@tex3d tex3d Feb 25, 2025

Choose a reason for hiding this comment

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

I noticed that below this point, there's:

    } else if (StoreInst *ST = dyn_cast<StoreInst>(U)) {

where it still scalarizes the store for the vector.

Did you mean to leave that scalarization in?

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 14, 2025

Choose a reason for hiding this comment

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

Yeah, I omitted store support in error. Clearly we need some testing for it as it never caused any issues.

}
LI->replaceAllUsesWith(NewVec);
LI->eraseFromParent();
Expand All @@ -228,12 +243,20 @@ void MatrixBitcastLowerPass::lowerMatrix(Instruction *M, Value *A) {
Value *V = ST->getValueOperand();
if (VectorType *Ty = dyn_cast<VectorType>(V->getType())) {
IRBuilder<> Builder(LI);
Value *zeroIdx = Builder.getInt32(0);
unsigned vecSize = Ty->getNumElements();
for (unsigned i = 0; i < vecSize; i++) {
Value *GEP = CreateEltGEP(A, i, zeroIdx, Builder);
Value *Elt = Builder.CreateExtractElement(V, i);
Builder.CreateStore(Elt, GEP);
if (SupportsVectors && Ty->getVectorNumElements() > 1) {
// Create a replacement store using the vector pointer.
Instruction *NewSt = ST->clone();
unsigned VecIdx = NewSt->getNumOperands() - 1;
NewSt->setOperand(VecIdx, A);
Builder.Insert(NewSt);
} else {
Value *zeroIdx = Builder.getInt32(0);
unsigned vecSize = Ty->getNumElements();
for (unsigned i = 0; i < vecSize; i++) {
Value *GEP = CreateEltGEP(A, i, zeroIdx, Builder);
Value *Elt = Builder.CreateExtractElement(V, i);
Builder.CreateStore(Elt, GEP);
}
}
ST->eraseFromParent();
} else {
Expand Down
3 changes: 3 additions & 0 deletions lib/HLSL/HLModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,9 @@ MDTuple *HLModule::EmitHLResources() {

void HLModule::LoadHLResources(const llvm::MDOperand &MDO) {
const llvm::MDTuple *pSRVs, *pUAVs, *pCBuffers, *pSamplers;
// No resources. Nothing to do.
if (MDO.get() == nullptr)
return;
m_pMDHelper->GetDxilResources(MDO, pSRVs, pUAVs, pCBuffers, pSamplers);

// Load SRV records.
Expand Down
43 changes: 34 additions & 9 deletions lib/Transforms/Scalar/LowerTypePasses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//

#include "dxc/DXIL/DxilConstants.h"
#include "dxc/DXIL/DxilModule.h"
#include "dxc/DXIL/DxilOperations.h"
#include "dxc/DXIL/DxilUtil.h"
#include "dxc/HLSL/HLModule.h"
Expand Down Expand Up @@ -180,10 +181,12 @@ bool LowerTypePass::runOnModule(Module &M) {
namespace {
class DynamicIndexingVectorToArray : public LowerTypePass {
bool ReplaceAllVectors;
bool SupportsVectors;

public:
explicit DynamicIndexingVectorToArray(bool ReplaceAll = false)
: LowerTypePass(ID), ReplaceAllVectors(ReplaceAll) {}
: LowerTypePass(ID), ReplaceAllVectors(ReplaceAll),
SupportsVectors(false) {}
static char ID; // Pass identification, replacement for typeid
void applyOptions(PassOptions O) override;
void dumpConfig(raw_ostream &OS) override;
Expand All @@ -194,6 +197,7 @@ class DynamicIndexingVectorToArray : public LowerTypePass {
Type *lowerType(Type *Ty) override;
Constant *lowerInitVal(Constant *InitVal, Type *NewTy) override;
StringRef getGlobalPrefix() override { return ".v"; }
void initialize(Module &M) override;

private:
bool HasVectorDynamicIndexing(Value *V);
Expand All @@ -207,6 +211,21 @@ class DynamicIndexingVectorToArray : public LowerTypePass {
void ReplaceAddrSpaceCast(ConstantExpr *CE, Value *A, IRBuilder<> &Builder);
};

void DynamicIndexingVectorToArray::initialize(Module &M) {
// Can be invoked in a few places:
// - From standard compile before dxilgen.
// - When linking, where dxmodule is available.
// - In isolated dxopt, where the module will need to be created.
// Since HL module can't be created when linking, check for that first.
// Otherwise, either retrieve or generate the HL module.
Copy link
Copy Markdown
Collaborator

@bogner bogner Mar 24, 2025

Choose a reason for hiding this comment

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

Should this say "Since dxmodule can't be created when linking..."?

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, we actually can create a dxil module when linking. Maybe I'll replace it with getorcreatedxilmodule just in case. We can't create an hlmodule while linking

Copy link
Copy Markdown
Collaborator Author

@pow2clk pow2clk Mar 25, 2025

Choose a reason for hiding this comment

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

I see why it's confusing now though. the "it" was supposed to be a dxil module, but that is not at all clear from the wording.

if (M.HasDxilModule()) {
SupportsVectors = M.GetDxilModule().GetShaderModel()->IsSM69Plus();
} else {
HLModule &HLM = M.GetOrCreateHLModule();
SupportsVectors = HLM.GetShaderModel()->IsSM69Plus();
Copy link
Copy Markdown
Contributor

@tex3d tex3d Mar 25, 2025

Choose a reason for hiding this comment

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

I mentioned earlier, it might be better to get the DXIL version from metadata without relying on full HLModule/DxilModule metadata loading. Maybe we can add a dxilutil::ReadDxilVersion function?

But the fixes to the HLModule loading are appreciated!

}
}

void DynamicIndexingVectorToArray::applyOptions(PassOptions O) {
GetPassOptionBool(O, "ReplaceAllVectors", &ReplaceAllVectors,
ReplaceAllVectors);
Expand Down Expand Up @@ -306,9 +325,21 @@ void DynamicIndexingVectorToArray::ReplaceStaticIndexingOnVector(Value *V) {
}

bool DynamicIndexingVectorToArray::needToLower(Value *V) {
bool MustReplaceVector = ReplaceAllVectors;
Type *Ty = V->getType()->getPointerElementType();
if (dyn_cast<VectorType>(Ty)) {
if (isa<GlobalVariable>(V) || ReplaceAllVectors) {

if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {
// Array must be replaced even without dynamic indexing to remove vector
// type in dxil.
MustReplaceVector = true;
Ty = dxilutil::GetArrayEltTy(AT);
}

if (isa<VectorType>(Ty)) {
// Only needed for 2+ vectors where native vectors unsupported.
if (SupportsVectors && Ty->getVectorNumElements() > 1)
return false;
if (isa<GlobalVariable>(V) || MustReplaceVector) {
return true;
}
// Don't lower local vector which only static indexing.
Expand All @@ -319,12 +350,6 @@ bool DynamicIndexingVectorToArray::needToLower(Value *V) {
ReplaceStaticIndexingOnVector(V);
return false;
}
} else if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {
// Array must be replaced even without dynamic indexing to remove vector
// type in dxil.
// TODO: optimize static array index in later pass.
Type *EltTy = dxilutil::GetArrayEltTy(AT);
return isa<VectorType>(EltTy);
}
return false;
}
Expand Down
Loading