Skip to content

[NFC] Express GPU texture formats via CPUBuffer GpuFormat field #1327

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

Open
alsepkow wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[NFC] Express GPU texture formats via CPUBuffer GpuFormat field #1327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a7a77a
[NFC] Express GPU texture formats via CPUBuffer GpuFormat field
alsepkow Jun 22, 2026
82cf91b
Simplify DX format selection by using FormatOrErr directly
alsepkow Jun 22, 2026
49c7a1a
Rename GpuFormat to GPUFormat to match CPU/GPU acronym casing
alsepkow Jun 23, 2026
0e383ea
Use truthy optional check for IsDepth instead of has_value()
alsepkow Jun 23, 2026
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
7 changes: 0 additions & 7 deletions include/API/FormatConversion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,6 @@ inline llvm::Expected<Format> toFormat(DataFormat Format, int Channels) {
return Format::RGBA32Float;
}
break;
case DataFormat::Depth32:
// D32FloatS8Uint is not expressible as DataFormat + Channels because the
// stencil component is uint8, not a second Depth32 channel. Once the
// pipeline uses Format directly, this limitation goes away.
if (Channels == 1)
return Format::D32Float;
break;
case DataFormat::UInt64:
// Only 1 and 2 channels of 64-bit integers are supported.
switch (Channels) {
Expand Down
8 changes: 5 additions & 3 deletions include/Support/Pipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ enum class DataFormat {
Float16,
Float32,
Float64,
Depth32,
Bool,
};

Expand Down Expand Up @@ -198,7 +197,6 @@ static inline uint32_t getFormatSize(DataFormat Format) {
case DataFormat::UInt32:
case DataFormat::Int32:
case DataFormat::Float32:
case DataFormat::Depth32:
case DataFormat::Bool:
return 4;
case DataFormat::Hex64:
Expand All @@ -216,6 +214,11 @@ struct CPUBuffer {
int Channels;
int Stride;
uint32_t ArraySize;
// When set, names the GPU texture format directly (e.g. D32Float) instead of
// inferring it from DataFormat + Channels via toFormat(). This lets depth
// buffers and other special formats be expressed without extending
// DataFormat.
std::optional<offloadtest::Format> GPUFormat;

@manon-traverse manon-traverse Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I love this change. In a future PR we should completely get rid of the old way of doing formats and just use the ones that are actually GPU API compatible.

// Data can contain one block of data for a singular resource
// or multiple blocks for a resource array.
llvm::SmallVector<std::unique_ptr<char[]>> Data;
Expand Down Expand Up @@ -931,7 +934,6 @@ template <> struct ScalarEnumerationTraits<offloadtest::DataFormat> {
ENUM_CASE(Float16);
ENUM_CASE(Float32);
ENUM_CASE(Float64);
ENUM_CASE(Depth32);
ENUM_CASE(Bool);
#undef ENUM_CASE
}
Expand Down
5 changes: 4 additions & 1 deletion lib/API/DX/Device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2559,7 +2559,10 @@ class DXDevice : public offloadtest::Device {
"Multiple mip levels are not yet "
"supported for DirectX textures.");

auto FormatOrErr = toFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
auto FormatOrErr =
R.BufferPtr->GPUFormat
? llvm::Expected<Format>(*R.BufferPtr->GPUFormat)
: toFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
Comment on lines -2562 to +2565

@bogner bogner Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be more sound to update toFormat to take all three of the GPUFormat, Format, and Channels and reconcile them. This way we can at least assert if we've somehow specified both in an incompatible way.

if (!FormatOrErr)
return FormatOrErr.takeError();

Expand Down
38 changes: 18 additions & 20 deletions lib/API/VK/Device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,6 @@ static VkFormat getVKFormat(DataFormat Format, int Channels) {
VKFormats(UINT, 64) break;
case DataFormat::Float64:
VKFormats(SFLOAT, 64) break;
case DataFormat::Depth32:
if (Channels != 1)
llvm_unreachable("Depth32 format only supports a single channel.");
return VK_FORMAT_D32_SFLOAT;
default:
llvm_unreachable("Unsupported Resource format specified");
}
Expand Down Expand Up @@ -3395,13 +3391,15 @@ class VulkanDevice : public offloadtest::Device {
llvm::Expected<ResourceRef> createImage(Resource &R, BufferRef &Host,
int UsageOverride = 0) {
const offloadtest::CPUBuffer &B = *R.BufferPtr;
if (B.Format == DataFormat::Depth32 && R.isReadWrite())
const bool IsDepth = B.GPUFormat && isDepthFormat(*B.GPUFormat);
if (IsDepth && R.isReadWrite())
return llvm::createStringError(std::errc::invalid_argument,
"Image memory allocation failed.");
VkImageCreateInfo ImageCreateInfo = {};
ImageCreateInfo.sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO;
ImageCreateInfo.imageType = getVKImageType(R.Kind);
ImageCreateInfo.format = getVKFormat(B.Format, B.Channels);
ImageCreateInfo.format = B.GPUFormat ? getVulkanFormat(*B.GPUFormat)
: getVKFormat(B.Format, B.Channels);
Comment on lines +3401 to +3402

@bogner bogner Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similarly here, if we update getVKFormat to reconcile the specification first then we don't need to have all of these places with their own checks that need to be aware of which way we've specified the format

ImageCreateInfo.mipLevels = B.OutputProps.MipLevels;
ImageCreateInfo.arrayLayers = 1;
ImageCreateInfo.samples = VK_SAMPLE_COUNT_1_BIT;
Expand Down Expand Up @@ -3823,12 +3821,14 @@ class VulkanDevice : public offloadtest::Device {
ViewCreateInfo.sType = VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO;
ViewCreateInfo.viewType = getImageViewType(R.Kind);
ViewCreateInfo.format =
getVKFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
R.BufferPtr->GPUFormat
? getVulkanFormat(*R.BufferPtr->GPUFormat)
: getVKFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
ViewCreateInfo.components = {
VK_COMPONENT_SWIZZLE_R, VK_COMPONENT_SWIZZLE_G,
VK_COMPONENT_SWIZZLE_B, VK_COMPONENT_SWIZZLE_A};
ViewCreateInfo.subresourceRange.aspectMask =
R.BufferPtr->Format == DataFormat::Depth32
(R.BufferPtr->GPUFormat && isDepthFormat(*R.BufferPtr->GPUFormat))
? VK_IMAGE_ASPECT_DEPTH_BIT
: VK_IMAGE_ASPECT_COLOR_BIT;
ViewCreateInfo.subresourceRange.baseMipLevel = 0;
Expand Down Expand Up @@ -4086,13 +4086,13 @@ class VulkanDevice : public offloadtest::Device {
return;
if (R.isImage()) {
const offloadtest::CPUBuffer &B = *R.BufferPtr;
const bool IsDepth = B.GPUFormat && isDepthFormat(*B.GPUFormat);
llvm::SmallVector<VkBufferImageCopy> Regions;
uint64_t CurrentOffset = 0;
for (int I = 0; I < B.OutputProps.MipLevels; ++I) {
VkBufferImageCopy Region = {};
Region.imageSubresource.aspectMask = B.Format == DataFormat::Depth32
? VK_IMAGE_ASPECT_DEPTH_BIT
: VK_IMAGE_ASPECT_COLOR_BIT;
Region.imageSubresource.aspectMask =
IsDepth ? VK_IMAGE_ASPECT_DEPTH_BIT : VK_IMAGE_ASPECT_COLOR_BIT;
Region.imageSubresource.mipLevel = I;
Region.imageSubresource.baseArrayLayer = 0;
Region.imageSubresource.layerCount = 1;
Expand All @@ -4110,9 +4110,8 @@ class VulkanDevice : public offloadtest::Device {
}

VkImageSubresourceRange SubRange = {};
SubRange.aspectMask = B.Format == DataFormat::Depth32
? VK_IMAGE_ASPECT_DEPTH_BIT
: VK_IMAGE_ASPECT_COLOR_BIT;
SubRange.aspectMask =
IsDepth ? VK_IMAGE_ASPECT_DEPTH_BIT : VK_IMAGE_ASPECT_COLOR_BIT;
SubRange.baseMipLevel = 0;
SubRange.levelCount = B.OutputProps.MipLevels;
SubRange.layerCount = 1;
Expand Down Expand Up @@ -4232,10 +4231,10 @@ class VulkanDevice : public offloadtest::Device {
return;
if (R.isImage()) {
const offloadtest::CPUBuffer &B = *R.BufferPtr;
const bool IsDepth = B.GPUFormat && isDepthFormat(*B.GPUFormat);
VkImageSubresourceRange SubRange = {};
SubRange.aspectMask = B.Format == DataFormat::Depth32
? VK_IMAGE_ASPECT_DEPTH_BIT
: VK_IMAGE_ASPECT_COLOR_BIT;
SubRange.aspectMask =
IsDepth ? VK_IMAGE_ASPECT_DEPTH_BIT : VK_IMAGE_ASPECT_COLOR_BIT;
SubRange.baseMipLevel = 0;
SubRange.levelCount = B.OutputProps.MipLevels;
SubRange.layerCount = 1;
Expand All @@ -4262,9 +4261,8 @@ class VulkanDevice : public offloadtest::Device {
uint64_t CurrentOffset = 0;
for (int I = 0; I < B.OutputProps.MipLevels; ++I) {
VkBufferImageCopy Region = {};
Region.imageSubresource.aspectMask = B.Format == DataFormat::Depth32
? VK_IMAGE_ASPECT_DEPTH_BIT
: VK_IMAGE_ASPECT_COLOR_BIT;
Region.imageSubresource.aspectMask =
IsDepth ? VK_IMAGE_ASPECT_DEPTH_BIT : VK_IMAGE_ASPECT_COLOR_BIT;
Region.imageSubresource.mipLevel = I;
Region.imageSubresource.baseArrayLayer = 0;
Region.imageSubresource.layerCount = 1;
Expand Down
8 changes: 2 additions & 6 deletions lib/Support/Check.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@ testBufferFloat(std::function<bool(const T &, const T &)> ComparisonFn,
case offloadtest::DataFormat::Float64:
return testAllArray<double>(ComparisonFn, B1, B2);
case offloadtest::DataFormat::Float32:
case offloadtest::DataFormat::Depth32:
return testAllArray<float>(ComparisonFn, B1, B2);
case offloadtest::DataFormat::Float16: {
return testAllArray<uint16_t>(ComparisonFn, B1, B2);
Expand All @@ -250,8 +249,7 @@ static bool testBufferFloatEpsilon(offloadtest::CPUBuffer *B1,
};
return testBufferFloat<double>(Fn, B1, B2);
}
case offloadtest::DataFormat::Float32:
case offloadtest::DataFormat::Depth32: {
case offloadtest::DataFormat::Float32: {
auto Fn = [Epsilon, DM](const float &FS, const float &FR) {
return compareFloatEpsilon(FS, FR, (float)Epsilon, DM);
};
Expand Down Expand Up @@ -280,8 +278,7 @@ static bool testBufferFloatULP(offloadtest::CPUBuffer *B1,
};
return testBufferFloat<double>(Fn, B1, B2);
}
case offloadtest::DataFormat::Float32:
case offloadtest::DataFormat::Depth32: {
case offloadtest::DataFormat::Float32: {
auto Fn = [ULPT, DM](const float &FS, const float &FR) {
return compareFloatULP(FS, FR, ULPT, DM);
};
Expand Down Expand Up @@ -379,7 +376,6 @@ static const std::string getBufferStr(offloadtest::CPUBuffer *B) {
case DF::Float16:
return formatBuffer<llvm::yaml::Hex16>(B); // assuming no native float16
case DF::Float32:
case DF::Depth32:
return formatBuffer<float>(B);
case DF::Float64:
return formatBuffer<double>(B);
Expand Down
6 changes: 1 addition & 5 deletions lib/Support/Pipeline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ void MappingTraits<offloadtest::CPUBuffer>::mapping(IO &I,
I.mapRequired("Name", B.Name);
I.mapRequired("Format", B.Format);
I.mapOptional("Channels", B.Channels, 1);
I.mapOptional("GPUFormat", B.GPUFormat);
I.mapOptional("Stride", B.Stride, 0);
I.mapOptional("ArraySize", B.ArraySize, 1);
setCounters(I, B);
Expand Down Expand Up @@ -407,9 +408,6 @@ void MappingTraits<offloadtest::CPUBuffer>::mapping(IO &I,
case DF::Float32:
setData<float>(I, B);
break;
case DF::Depth32:
setData<float>(I, B);
break;
case DF::Float64:
setData<double>(I, B);
break;
Expand Down Expand Up @@ -528,8 +526,6 @@ void MappingTraits<offloadtest::PushConstantValue>::mapping(
return setData<llvm::yaml::Hex16>(I, B); // assuming no native float16
case DF::Float32:
return setData<float>(I, B);
case DF::Depth32:
return setData<float>(I, B);
case DF::Float64:
return setData<double>(I, B);
case DF::Bool:
Expand Down
3 changes: 2 additions & 1 deletion test/Feature/Textures/Texture2D.GatherCmp.test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ Shaders:

Buffers:
- Name: Tex
Format: Depth32
Format: Float32
GPUFormat: D32Float
Comment on lines +71 to +72

@bogner bogner Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the Format: Float32 just entirely ignored here since GPUFormat is specified? This feels a bit confusing.

I guess it isn't ignored but it's how we're interpreting the values in the YAML?

Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2, # (0,0) R=0.2
Expand Down
3 changes: 2 additions & 1 deletion test/Feature/Textures/Texture2D.SampleCmp.test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ Shaders:

Buffers:
- Name: Tex
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1

@inbelic inbelic Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC, if GpuFormat is specified then both Format and Channels are ignored. Is there any easy way to represent that in the struct/yaml so they are not silently ignored. Maybe a std::variant works?

@alsepkow alsepkow Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Your general intuition for the std::variant makes sense. But GPUFormat only implies that Channels are ignored. Format is still used for how we interpret the CPU bytes.

@manon-traverse manon-traverse Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why wouldn't we be able to also use GPUFormat to interpret the human-readable data when reading the YAML?

OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2, # (0,0) -> 0.2
Expand Down
9 changes: 6 additions & 3 deletions test/Feature/Vk.SampledTextures/Vk.SampledTexture2D/Vk.SampledTexture2D.GatherCmp.test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ Shaders:

Buffers:
- Name: SampledTexLess
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2, # (0,0) R=0.2
Expand All @@ -58,7 +59,8 @@ Buffers:
0.8 ] # (1,1) R=0.8

- Name: SampledTexGreater
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2,
Expand All @@ -67,7 +69,8 @@ Buffers:
0.8 ]

- Name: SampledTexRepeat
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2,
Expand Down
6 changes: 4 additions & 2 deletions test/Feature/Vk.SampledTextures/Vk.SampledTexture2D/Vk.SampledTexture2D.SampleCmp.test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ Shaders:

Buffers:
- Name: SampledTexLess
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2, # (0,0) -> 0.2
Expand All @@ -97,7 +98,8 @@ Buffers:
0.8 ] # (1,1) -> 0.8

- Name: SampledTexGreater
Format: Depth32
Format: Float32
GPUFormat: D32Float
Channels: 1
OutputProps: { Width: 2, Height: 2, Depth: 1 }
Data: [ 0.2,
Expand Down
Loading