Skip to content

Commit 71aa195

Browse files
alsepkowCopilot
andauthored
Revert out-of-scope DxilGenerationPass changes from #8274 (#8321)
## Summary Reverts the `DxilGenerationPass` ByteAddressBuffer scalar store changes and removes scalar store tests that were included in PR #8274. These changes were out of scope for the vector load/store fix and, on further discussion, were concluded to be incomplete. Follow-up issue for the proper fix: #8322 ## What changed - **DxilGenerationPass.cpp**: Reverted the ByteAddressBuffer fallback path added in `ReplaceMinPrecisionRawBufferStoreByType`. The `sext` fallback was wrong for `min16uint` (loses signedness), and the fix belongs in CodeGen where signedness info is still available. - **min_precision_raw_load_store.hlsl**: Removed scalar load/store tests. Scalar `ByteAddressBuffer::Store<min16int>()` hits a pre-existing crash in `TranslateMinPrecisionRawBuffer` (`cast<StructType>` on ByteAddressBuffer's `i32` inner element). Test now covers vector loads/stores only, which is the scope of the original fix. ## Context The original PR #8274 correctly fixed `RawBufferVectorLoad/Store` to widen min precision types to 32-bit. However, it also added a ByteAddressBuffer scalar store fix in `DxilGenerationPass` that: 1. Crept outside the scope of the vector load/store fix 2. Was incomplete — the `sext` fallback is wrong for unsigned types (`min16uint`) 3. Should instead be handled during Clang CodeGen, where signedness information is available Scalar ByteAddressBuffer template store widening for min precision types is a separate pre-existing issue that needs a proper fix in CodeGen. Co-authored-by: Copilot <[email protected]>
1 parent a529a31 commit 71aa195

2 files changed

Lines changed: 31 additions & 70 deletions

File tree

lib/HLSL/DxilGenerationPass.cpp

Lines changed: 30 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -993,10 +993,11 @@ void ReplaceMinPrecisionRawBufferStoreByType(
993993
Args.emplace_back(NewV);
994994
}
995995
} else if (FromTy->isIntegerTy()) {
996-
// Since we are extending integer, we have to know if we should sign ext
997-
// or zero ext. For StructuredBuffers we get signedness from the struct
998-
// type annotation. For ByteAddressBuffer (raw buffers) there is no struct
999-
// annotation, so we fall back to sext as a conservative default.
996+
// This case only applies to typed buffer since Store operation of byte
997+
// address buffer for min precision is handled by implicit conversion on
998+
// intrinsic call. Since we are extending integer, we have to know if we
999+
// should sign ext or zero ext. We can do this by iterating checking the
1000+
// size of the element at struct type and comp type at type annotation
10001001
CallInst *handleCI = dyn_cast<CallInst>(
10011002
CI->getArgOperand(DxilInst_RawBufferStore::arg_uav));
10021003
DXASSERT(handleCI,
@@ -1006,50 +1007,34 @@ void ReplaceMinPrecisionRawBufferStoreByType(
10061007
"otherwise fail to handle for buffer store lost its retTy");
10071008
StructType *STy = dyn_cast<StructType>(resTyIt->second);
10081009

1009-
StructType *InnerSTy =
1010-
STy ? dyn_cast<StructType>(STy->getElementType(0)) : nullptr;
1011-
DxilStructAnnotation *SAnnot =
1012-
InnerSTy ? typeSys.GetStructAnnotation(InnerSTy) : nullptr;
1013-
1014-
if (SAnnot) {
1015-
// StructuredBuffer path: use struct annotation to determine signedness.
1016-
ConstantInt *offsetInt = dyn_cast<ConstantInt>(
1017-
CI->getArgOperand(DxilInst_RawBufferStore::arg_elementOffset));
1018-
unsigned offset = offsetInt->getSExtValue();
1019-
unsigned currentOffset = 0;
1020-
for (DxilStructTypeIterator iter = begin(InnerSTy, SAnnot),
1021-
ItEnd = end(InnerSTy, SAnnot);
1022-
iter != ItEnd; ++iter) {
1023-
std::pair<Type *, DxilFieldAnnotation *> pair = *iter;
1024-
currentOffset += DL.getTypeAllocSize(pair.first);
1025-
if (currentOffset > offset) {
1026-
if (pair.second->GetCompType().IsUIntTy()) {
1027-
for (unsigned i = 4; i < 8; ++i) {
1028-
Value *NewV = CIBuilder.CreateZExt(CI->getArgOperand(i), ToTy);
1029-
Args.emplace_back(NewV);
1030-
}
1031-
break;
1032-
} else if (pair.second->GetCompType().IsIntTy()) {
1033-
for (unsigned i = 4; i < 8; ++i) {
1034-
Value *NewV = CIBuilder.CreateSExt(CI->getArgOperand(i), ToTy);
1035-
Args.emplace_back(NewV);
1036-
}
1037-
break;
1038-
} else {
1039-
DXASSERT(false, "Invalid comp type");
1010+
STy = cast<StructType>(STy->getElementType(0));
1011+
DxilStructAnnotation *SAnnot = typeSys.GetStructAnnotation(STy);
1012+
ConstantInt *offsetInt = dyn_cast<ConstantInt>(
1013+
CI->getArgOperand(DxilInst_RawBufferStore::arg_elementOffset));
1014+
unsigned offset = offsetInt->getSExtValue();
1015+
unsigned currentOffset = 0;
1016+
for (DxilStructTypeIterator iter = begin(STy, SAnnot),
1017+
ItEnd = end(STy, SAnnot);
1018+
iter != ItEnd; ++iter) {
1019+
std::pair<Type *, DxilFieldAnnotation *> pair = *iter;
1020+
currentOffset += DL.getTypeAllocSize(pair.first);
1021+
if (currentOffset > offset) {
1022+
if (pair.second->GetCompType().IsUIntTy()) {
1023+
for (unsigned i = 4; i < 8; ++i) {
1024+
Value *NewV = CIBuilder.CreateZExt(CI->getArgOperand(i), ToTy);
1025+
Args.emplace_back(NewV);
10401026
}
1027+
break;
1028+
} else if (pair.second->GetCompType().IsIntTy()) {
1029+
for (unsigned i = 4; i < 8; ++i) {
1030+
Value *NewV = CIBuilder.CreateSExt(CI->getArgOperand(i), ToTy);
1031+
Args.emplace_back(NewV);
1032+
}
1033+
break;
1034+
} else {
1035+
DXASSERT(false, "Invalid comp type");
10411036
}
10421037
}
1043-
} else {
1044-
// ByteAddressBuffer path: no struct annotation available, so
1045-
// signedness is unknown. Default to sext.
1046-
for (unsigned i = 4; i < 8; ++i) {
1047-
Value *Arg = CI->getArgOperand(i);
1048-
if (isa<UndefValue>(Arg))
1049-
Args.emplace_back(UndefValue::get(ToTy));
1050-
else
1051-
Args.emplace_back(CIBuilder.CreateSExt(Arg, ToTy));
1052-
}
10531038
}
10541039
}
10551040

tools/clang/test/HLSLFileCheck/hlsl/objects/ByteAddressBuffer/min_precision_raw_load_store.hlsl

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,33 +30,9 @@ void main() {
3030
// CHECK: call void @dx.op.rawBufferVectorStore.v3f32
3131
g_buf.Store< min16float3 >(60, vf);
3232

33-
// === Scalar loads/stores (RawBufferLoad/Store) ===
34-
35-
// min16int scalar: should use i32 rawBufferStore
36-
// CHECK: call %dx.types.ResRet.i32 @dx.op.rawBufferLoad.i32
37-
min16int si = g_buf.Load< min16int >(72);
38-
// CHECK: call void @dx.op.rawBufferStore.i32
39-
g_buf.Store< min16int >(76, si);
40-
41-
// min16uint scalar: should use i32 rawBufferStore
42-
// CHECK: call %dx.types.ResRet.i32 @dx.op.rawBufferLoad.i32
43-
min16uint su = g_buf.Load< min16uint >(80);
44-
// CHECK: call void @dx.op.rawBufferStore.i32
45-
g_buf.Store< min16uint >(84, su);
46-
47-
// min16float scalar: should use f32 rawBufferStore
48-
// CHECK: call %dx.types.ResRet.f32 @dx.op.rawBufferLoad.f32
49-
min16float sf = g_buf.Load< min16float >(88);
50-
// CHECK: call void @dx.op.rawBufferStore.f32
51-
g_buf.Store< min16float >(92, sf);
52-
53-
// Verify i16/f16 ops are NOT used.
33+
// Verify i16/f16 ops are NOT used for vector loads/stores.
5434
// CHECK-NOT: rawBufferVectorLoad.v{{[0-9]+}}i16
5535
// CHECK-NOT: rawBufferVectorStore.v{{[0-9]+}}i16
5636
// CHECK-NOT: rawBufferVectorLoad.v{{[0-9]+}}f16
5737
// CHECK-NOT: rawBufferVectorStore.v{{[0-9]+}}f16
58-
// CHECK-NOT: rawBufferLoad.i16
59-
// CHECK-NOT: rawBufferStore.i16
60-
// CHECK-NOT: rawBufferLoad.f16
61-
// CHECK-NOT: rawBufferStore.f16
6238
}

0 commit comments

Comments
 (0)