-
Notifications
You must be signed in to change notification settings - Fork 854
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.
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
Changes from all commits
68a284b
556f6e6
654bf8a
248fe80
f1de617
47d42f0
d480caa
c2ee61b
a3a39b8
6f7f9ec
1596d99
411b921
8dc2a3e
46829dd
9002bee
ba831c0
7292edb
96ac2e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
|
@@ -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();) { | ||
|
|
@@ -185,18 +188,19 @@ void MatrixBitcastLowerPass::lowerMatrix(Instruction *M, Value *A) { | |
| 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); | ||
| DXASSERT(GEP->user_empty(), "else lower matrix fail"); | ||
|
|
@@ -211,13 +215,23 @@ void MatrixBitcastLowerPass::lowerMatrix(Instruction *M, Value *A) { | |
| } 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); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
|
@@ -228,12 +242,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 { | ||
|
|
||
There was a problem hiding this comment.
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 checkingIsSM69Plus()instead of always returning true?There was a problem hiding this comment.
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
The inclusion here adds them to the list of instructions that are approved for general validation by
IsLLVMInstructionAllowed, a generated function that merelyreturns 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
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.