[NFC] Express GPU texture formats via CPUBuffer GpuFormat field#1327
[NFC] Express GPU texture formats via CPUBuffer GpuFormat field#1327alsepkow wants to merge 4 commits into
Conversation
alsepkow
force-pushed
the
alex/nfc-gpuformat
branch
from
61b7687 to
fcb8406
Compare
Replace the special-cased DataFormat::Depth32 enum value with a general CPUBuffer::GpuFormat field that names a GPU Format directly (e.g. D32Float), instead of inferring it from DataFormat + Channels via toFormat(). This removes the need to extend DataFormat for every special texture format and lets depth-format textures be expressed as a regular float buffer plus an explicit GpuFormat. The Vulkan backend now selects the image/view format and depth/color aspect from GpuFormat; the DX and Check paths drop the now-unused Depth32 cases. Existing GatherCmp/SampleCmp depth tests are migrated from "Format: Depth32" to "Format: Float32" + "GpuFormat: D32Float". No functional change intended. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alsepkow
force-pushed
the
alex/nfc-gpuformat
branch
from
fcb8406 to
3a7a77a
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| Format: Depth32 | ||
| Format: Float32 | ||
| GpuFormat: D32Float | ||
| Channels: 1 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why wouldn't we be able to also use GPUFormat to interpret the human-readable data when reading the YAML?
Addresses review feedback on PR llvm#1327: use all-caps GPU acronym to match CPUBuffer naming, across the CPUBuffer field, the YAML key, and tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on PR llvm#1327: prefer 'B.GPUFormat && isDepthFormat(*B.GPUFormat)' for consistency with the existing image-view check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // 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; |
There was a problem hiding this comment.
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.
| Format: Depth32 | ||
| Format: Float32 | ||
| GpuFormat: D32Float | ||
| Channels: 1 |
There was a problem hiding this comment.
Why wouldn't we be able to also use GPUFormat to interpret the human-readable data when reading the YAML?
bogner
left a comment
bogner
left a comment
There was a problem hiding this comment.
I worry that this change as is makes things pretty confusing. There are now two slightly incompatible ways to specify the format, and as far as I can tell nothing stops you from doing both.
I realize it would be a massive change to all of the tests to just switch to GPUFormat entirely, but if this is the direction we're going I think we might be better off just ripping off the bandaid and changing it all.
| Format: Float32 | ||
| GPUFormat: D32Float |
There was a problem hiding this comment.
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?
| 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); |
There was a problem hiding this comment.
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.
| ImageCreateInfo.format = B.GPUFormat ? getVulkanFormat(*B.GPUFormat) | ||
| : getVKFormat(B.Format, B.Channels); |
There was a problem hiding this comment.
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
4 participants
Resolves #1326.
Replaces the special-cased
DataFormat::Depth32with an optionalCPUBuffer::GpuFormatfield that names a GPUFormatdirectly. The Vulkan backend now derives the image/view format and depth/color aspect fromGpuFormat; theCheckpath drops the unusedDepth32cases. ExistingGatherCmp/SampleCmpdepth tests migrate fromFormat: Depth32toFormat: Float32+GpuFormat: D32Float.See #1326 for full motivation.