[DirectX] Fix the writing of ConstantExpr GEPs to DXIL bitcode #154446
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-directx Author: Deric C. (Icohedron) ChangesFixes #153304 Changes:
Full diff: https://github.com/llvm/llvm-project/pull/154446.diff 7 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILDataScalarization.cpp b/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
index feecfc0880e25..5eb6382e2177a 100644
--- a/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
+++ b/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
@@ -153,6 +153,8 @@ bool DataScalarizerVisitor::visitLoadInst(LoadInst &LI) {
NewLoad->setAlignment(LI.getAlign());
LI.replaceAllUsesWith(NewLoad);
LI.eraseFromParent();
+ if (CE->use_empty())
+ CE->destroyConstant();
visitGetElementPtrInst(*OldGEP);
return true;
}
@@ -173,6 +175,8 @@ bool DataScalarizerVisitor::visitStoreInst(StoreInst &SI) {
NewStore->setAlignment(SI.getAlign());
SI.replaceAllUsesWith(NewStore);
SI.eraseFromParent();
+ if (CE->use_empty())
+ CE->destroyConstant();
visitGetElementPtrInst(*OldGEP);
return true;
}
@@ -343,7 +347,7 @@ bool DataScalarizerVisitor::visitGetElementPtrInst(GetElementPtrInst &GEPI) {
GOp->replaceAllUsesWith(NewGEP);
- if (auto *CE = dyn_cast<ConstantExpr>(GOp))
+ if (auto *CE = dyn_cast<ConstantExpr>(GOp); CE && CE->use_empty())
CE->destroyConstant();
else if (auto *OldGEPI = dyn_cast<GetElementPtrInst>(GOp))
OldGEPI->eraseFromParent();
diff --git a/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp b/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
index 7e1436e05a34a..9ab236a9c367d 100644
--- a/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
+++ b/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
@@ -166,6 +166,8 @@ bool DXILFlattenArraysVisitor::visitLoadInst(LoadInst &LI) {
NewLoad->setAlignment(LI.getAlign());
LI.replaceAllUsesWith(NewLoad);
LI.eraseFromParent();
+ if (CE->use_empty())
+ CE->destroyConstant();
visitGetElementPtrInst(*OldGEP);
return true;
}
@@ -188,6 +190,8 @@ bool DXILFlattenArraysVisitor::visitStoreInst(StoreInst &SI) {
NewStore->setAlignment(SI.getAlign());
SI.replaceAllUsesWith(NewStore);
SI.eraseFromParent();
+ if (CE->use_empty())
+ CE->destroyConstant();
visitGetElementPtrInst(*OldGEP);
return true;
}
@@ -243,6 +247,8 @@ bool DXILFlattenArraysVisitor::visitGetElementPtrInst(GetElementPtrInst &GEP) {
GEP.replaceAllUsesWith(NewGEPI);
GEP.eraseFromParent();
+ if (PtrOpGEPCE->use_empty())
+ PtrOpGEPCE->destroyConstant();
visitGetElementPtrInst(*OldGEPI);
visitGetElementPtrInst(*NewGEPI);
return true;
diff --git a/llvm/lib/Target/DirectX/DXILLegalizePass.cpp b/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
index 3427968d199f6..515f64a4d726d 100644
--- a/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
+++ b/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
@@ -143,6 +143,10 @@ static bool fixI8UseChain(Instruction &I,
LoadInst *NewLoad = Builder.CreateLoad(ElementType, NewGEP);
ReplacedValues[Load] = NewLoad;
Load->replaceAllUsesWith(NewLoad);
+ Load->setOperand(Load->getPointerOperandIndex(),
+ PoisonValue::get(CE->getType()));
+ if (CE->use_empty())
+ CE->destroyConstant();
ToRemove.push_back(Load);
return true;
}
diff --git a/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp b/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
index 1d79c3018439e..bc1a3a7995bda 100644
--- a/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
+++ b/llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp
@@ -2113,7 +2113,7 @@ void DXILBitcodeWriter::writeConstants(unsigned FirstVal, unsigned LastVal,
}
break;
case Instruction::GetElementPtr: {
- Code = bitc::CST_CODE_CE_GEP;
+ Code = bitc::CST_CODE_CE_GEP_OLD;
const auto *GO = cast<GEPOperator>(C);
if (GO->isInBounds())
Code = bitc::CST_CODE_CE_INBOUNDS_GEP;
diff --git a/llvm/lib/Target/DirectX/DirectXIRPasses/PointerTypeAnalysis.cpp b/llvm/lib/Target/DirectX/DirectXIRPasses/PointerTypeAnalysis.cpp
index f99bb4f4eaee1..b996075e31ad2 100644
--- a/llvm/lib/Target/DirectX/DirectXIRPasses/PointerTypeAnalysis.cpp
+++ b/llvm/lib/Target/DirectX/DirectXIRPasses/PointerTypeAnalysis.cpp
@@ -15,25 +15,34 @@
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/Module.h"
+#include "llvm/IR/Operator.h"
using namespace llvm;
using namespace llvm::dxil;
namespace {
+Type *classifyFunctionType(const Function &F, PointerTypeMap &Map);
+
// Classifies the type of the value passed in by walking the value's users to
// find a typed instruction to materialize a type from.
Type *classifyPointerType(const Value *V, PointerTypeMap &Map) {
assert(V->getType()->isPointerTy() &&
"classifyPointerType called with non-pointer");
+
+ // A CallInst will trigger this case, and we want to classify its Function
+ // operand as a Function rather than a generic Value.
+ if (const Function *F = dyn_cast<Function>(V))
+ return classifyFunctionType(*F, Map);
+
auto It = Map.find(V);
if (It != Map.end())
return It->second;
Type *PointeeTy = nullptr;
- if (auto *Inst = dyn_cast<GetElementPtrInst>(V)) {
- if (!Inst->getResultElementType()->isPointerTy())
- PointeeTy = Inst->getResultElementType();
+ if (auto *GEP = dyn_cast<GEPOperator>(V)) {
+ if (!GEP->getResultElementType()->isPointerTy())
+ PointeeTy = GEP->getResultElementType();
} else if (auto *Inst = dyn_cast<AllocaInst>(V)) {
PointeeTy = Inst->getAllocatedType();
} else if (auto *GV = dyn_cast<GlobalVariable>(V)) {
@@ -41,6 +50,11 @@ Type *classifyPointerType(const Value *V, PointerTypeMap &Map) {
}
for (const auto *User : V->users()) {
+ // This assert is here because there have been ConstantExpr GEPs with no
+ // users causing the pointer type analysis to misclassify the PointeeTy.
+ [[maybe_unused]] const Constant *C = dyn_cast<Constant>(User);
+ assert((!C || !C->use_empty()) && "A Constant should not have no uses");
+
Type *NewPointeeTy = nullptr;
if (const auto *Inst = dyn_cast<LoadInst>(User)) {
NewPointeeTy = Inst->getType();
@@ -49,8 +63,8 @@ Type *classifyPointerType(const Value *V, PointerTypeMap &Map) {
// When store value is ptr type, cannot get more type info.
if (NewPointeeTy->isPointerTy())
continue;
- } else if (const auto *Inst = dyn_cast<GetElementPtrInst>(User)) {
- NewPointeeTy = Inst->getSourceElementType();
+ } else if (const auto *GEP = dyn_cast<GEPOperator>(User)) {
+ NewPointeeTy = GEP->getSourceElementType();
}
if (NewPointeeTy) {
// HLSL doesn't support pointers, so it is unlikely to get more than one
@@ -204,6 +218,9 @@ PointerTypeMap PointerTypeAnalysis::run(const Module &M) {
for (const auto &I : B) {
if (I.getType()->isPointerTy())
classifyPointerType(&I, Map);
+ for (const auto &O : I.operands())
+ if (O.get()->getType()->isPointerTy())
+ classifyPointerType(O.get(), Map);
}
}
}
diff --git a/llvm/test/tools/dxil-dis/constantexpr-gep.ll b/llvm/test/tools/dxil-dis/constantexpr-gep.ll
new file mode 100644
index 0000000000000..56d46d071df7a
--- /dev/null
+++ b/llvm/test/tools/dxil-dis/constantexpr-gep.ll
@@ -0,0 +1,26 @@
+; RUN: llc --filetype=obj %s -o - | dxil-dis -o - | FileCheck %s
+target triple = "dxil-unknown-shadermodel6.7-library"
+
+; CHECK: [[GLOBAL:@.*]] = unnamed_addr addrspace(3) global [10 x i32] zeroinitializer, align 4
+@g = local_unnamed_addr addrspace(3) global [10 x i32] zeroinitializer, align 4
+
+define i32 @fn() #0 {
+; CHECK-LABEL: define i32 @fn()
+; CHECK-NEXT: [[LOAD:%.*]] = load i32, i32 addrspace(3)* getelementptr inbounds ([10 x i32], [10 x i32] addrspace(3)* [[GLOBAL]], i32 0, i32 1), align 4
+; CHECK-NEXT: ret i32 [[LOAD]]
+;
+ %gep = getelementptr [10 x i32], ptr addrspace(3) @g, i32 0, i32 1
+ %ld = load i32, ptr addrspace(3) %gep, align 4
+ ret i32 %ld
+}
+
+define i32 @fn2() #0 {
+; CHECK-LABEL: define i32 @fn2()
+; CHECK-NEXT: [[LOAD:%.*]] = load i32, i32 addrspace(3)* getelementptr inbounds ([10 x i32], [10 x i32] addrspace(3)* [[GLOBAL]], i32 0, i32 2), align 4
+; CHECK-NEXT: ret i32 [[LOAD]]
+;
+ %ld = load i32, ptr addrspace(3) getelementptr ([10 x i32], ptr addrspace(3) @g, i32 0, i32 2), align 4
+ ret i32 %ld
+}
+
+attributes #0 = { "hlsl.export" }
diff --git a/llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp b/llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp
index 9d41e94bb0bae..6ae139e076281 100644
--- a/llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp
+++ b/llvm/unittests/Target/DirectX/PointerTypeAnalysisTests.cpp
@@ -8,6 +8,7 @@
#include "DirectXIRPasses/PointerTypeAnalysis.h"
#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
@@ -123,6 +124,33 @@ TEST(PointerTypeAnalysis, DiscoverGEP) {
EXPECT_THAT(Map, Contains(Pair(IsA<GetElementPtrInst>(), I64Ptr)));
}
+TEST(PointerTypeAnalysis, DiscoverConstantExprGEP) {
+ StringRef Assembly = R"(
+ @g = internal global [10 x i32] zeroinitializer
+ define i32 @test() {
+ %i = load i32, ptr getelementptr ([10 x i32], ptr @g, i64 0, i64 1)
+ ret i32 %i
+ }
+ )";
+
+ LLVMContext Context;
+ SMDiagnostic Error;
+ auto M = parseAssemblyString(Assembly, Error, Context);
+ ASSERT_TRUE(M) << "Bad assembly?";
+
+ PointerTypeMap Map = PointerTypeAnalysis::run(*M);
+ ASSERT_EQ(Map.size(), 3u);
+ Type *I32Ty = Type::getInt32Ty(Context);
+ Type *I32Ptr = TypedPointerType::get(I32Ty, 0);
+ Type *I32ArrPtr = TypedPointerType::get(ArrayType::get(I32Ty, 10), 0);
+ Type *FnTy = FunctionType::get(I32Ty, {}, false);
+
+ EXPECT_THAT(Map, Contains(Pair(IsA<GlobalVariable>(), I32ArrPtr)));
+ EXPECT_THAT(Map,
+ Contains(Pair(IsA<Function>(), TypedPointerType::get(FnTy, 0))));
+ EXPECT_THAT(Map, Contains(Pair(IsA<ConstantExpr>(), I32Ptr)));
+}
+
TEST(PointerTypeAnalysis, TraceIndirect) {
StringRef Assembly = R"(
define i64 @test(ptr %p) {
|
farzonl
reviewed
Aug 21, 2025
| @@ -173,6 +175,8 @@ bool DataScalarizerVisitor::visitStoreInst(StoreInst &SI) { | |||
| NewStore->setAlignment(SI.getAlign()); | |||
| SI.replaceAllUsesWith(NewStore); | |||
| SI.eraseFromParent(); | |||
| if (CE->use_empty()) | |||
There was a problem hiding this comment.
Interesting so even though we erase the StoreInst the CE lives on? Is there maybe a bug in the ConstantExpr tracking? Is there something where we can do this once in a more generic way so we don't have to do that for every ConstExpr when we are deleting the instructions that use constExpr. does it make sense to maybe do something like this on the tail end of eraseFromParent?
There was a problem hiding this comment.
Hmm. I spent some time digging some more into it.
Supposedly it is intentional that Constants can linger around even after all its uses are eliminated.
There is a function called removeDeadConstantUsers that should be called before checking users of globals.
llvm-project/llvm/include/llvm/IR/Constant.h
Lines 201 to 205 in a3ed96b
I will reverse the change of all these manual if (CE->use_empty()) CE->destroyConstant(); in each pass, and just call GV->removeDeadConstantUsers() in the PointerTypeAnalysis instead.
farzonl
reviewed
Aug 21, 2025
|
Rest of the code looks good. I'm only concered about how we are handling ConstExpr. I feel like the way we are doing it isn't future proofing to the next pass that might operate on them and not know to delete them after instruction erasure. |
inbelic
reviewed
Aug 22, 2025
I changed the handling of ConstantExpr. Each pass no longer has the responsibility of cleaning up dead ConstantExprs. |
inbelic
approved these changes
Aug 26, 2025
farzonl
reviewed
Aug 26, 2025
| if (auto *CE = dyn_cast<ConstantExpr>(GOp)) | ||
| CE->destroyConstant(); | ||
| else if (auto *OldGEPI = dyn_cast<GetElementPtrInst>(GOp)) | ||
| if (auto *OldGEPI = dyn_cast<GetElementPtrInst>(GOp)) |
There was a problem hiding this comment.
A little confused what is going on here. In the diff it looks like we always did OldGEPI->eraseFromParent(); was this previously defined somewhere above? Is the diff off? Why do we have to dyn_cast GOp? Shouldn't that already be a GEP?
There was a problem hiding this comment.
GOp is indeed a GEPOperator, but it could be a ConstantExpr GEP or an Instruction.
The original implementation here checked if GOp was a ConstantExpr or an Instruction and then called destroyConstant or eraseFromParent respectively.
The change/diff is that destroyConstant is no longer called on GOp if it is a ConstantExpr. But eraseFromParent will still be called if GOp is an Instruction.
There was a problem hiding this comment.
ah I see this is clearer when you don't combine the +/- diffs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #153304
Changes:
ConstantExprGEPs to DXIL bitcode, the bitcode writer will use the old Constant CodeCST_CODE_CE_GEP_OLD = 12instead of the newerCST_CODE_CE_GEP = 32which is interpreted as an undef in DXIL. Additional context: CST_CODE_CE_GEP = 12 in DXC while the same constant code is labeled CST_CODE_CE_GEP_OLD in LLVMPointerTypeAnalysisto be able to analyze pointer-typed constants that appear in the operands of instructions so that the correct type of theConstantExprGEP is determined and written into the DXIL bitcode.PointerTypeAnalysistest and dxil-dis test to ensure that the pointer type ofConstantExprGEPs are resolved andConstantExprGEPs are written to DXIL bitcode correctlyIn addition, this PR also adds a missing call to
GV.removeDeadConstantUsers()in the DXILFinalizeLinkage pass, and removes an unnecessary manual removal of a ConstantExpr in the DXILFlattenArrays pass.