Skip to content
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

[SPIR-V] support loading "structured" data from a random device address #4289

Closed
jaebaek opened this issue Feb 22, 2022 · 12 comments
Closed
Labels
spirv Work related to SPIR-V

Comments

@jaebaek
Copy link
Collaborator

jaebaek commented Feb 22, 2022

We support loading "bytes" data from a random device address without a memory layout support in #4226.
In the future, we will add loading "structured" data from a random device address, which needs a sophisticated design for the memory layout rules.

@jaebaek jaebaek added the spirv Work related to SPIR-V label Feb 22, 2022
@BeastLe9enD
Copy link

Any news on this? It would be very interesting to be able to work with complicated structure types

@natevm
Copy link
Contributor

natevm commented Dec 22, 2022

It would be helpful to have a real world example here.

I don’t see why one would want any memory layout other than HLSL’s 16 byte alignment rules, especially if this raw buffer of data is a byte for byte copy from the host.

In CUDA for example, a device address (a pointer) leaves it to the user to get correct byte alignment, setup the appropriate structure layout etc. No strange layout rules apply there, and i’d argue CUDA is a fairly successful language for keeping thing simple like this.

I’d currently argue against such a feature, but perhaps there is some use case I’m not seeing?

@devshgraphicsprogramming

Or one could just follow the scalar layout rules, since the device is likely to have them if it has BDA anyway?

@devshgraphicsprogramming

Alternatively if the compiler instantiated "reference meta types" like suggested in #4986

then you could add the layout rules as a template parameter to later work out the OpAccessChain

@natevm
Copy link
Contributor

natevm commented Feb 1, 2023

Or one could just follow the scalar layout rules, since the device is likely to have them if it has BDA anyway?

I personally have been assuming scalar, which seems to work fine for the “bundles” platforms using this feature. For now this seems like a safe assumption… but I could see an argument made for the other layouts.

At the moment, I’ve noticed on AMD platforms that these raw buffer loads and stores cause their internal driver compiler to crash. Perhaps this has something to do with the layout rules?

@s-perron
Copy link
Collaborator

We will implement the vk::BufferPointer proposal. We expect that to be enough.

@s-perron s-perron closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2024
@github-project-automation github-project-automation bot moved this from For Google to Triaged in HLSL Triage Aug 23, 2024
@natevm
Copy link
Contributor

natevm commented Aug 23, 2024

@s-perron this is the second time over the past three years where Microsoft has swept a massive number of our issues under the rug as if they never existed.

If this company actually cared about these issues, they would be adding them as targets for unit tests, and not just closing them all in one day.

To anyone out there running into these issues, I cannot recommend switching to slang enough. Get the latest version of the compiler here! : https://github.com/shader-slang/slang/releases

@s-perron
Copy link
Collaborator

s-perron commented Sep 3, 2024

The implementation of vk::BufferPointer will be covered with #6489, so we do not need this issue. I should have mentioned that in the previous comment. That will give the behaviour that was requested in this issue.

Also, because this issue is not very detailed, I'm not sure what is still missing. You can load a struct using vk::RawBufferLoad: https://godbolt.org/z/ne66T75c4. However, that is not our long term direction.

@natevm
Copy link
Contributor

natevm commented Sep 3, 2024

@s-perron this is the main issue:

At the moment, I’ve noticed on AMD platforms that these raw buffer loads and stores cause their internal driver compiler to crash.

In slang, I don’t crash on AMD, so I believe this is a dxc compiler bug rather than a driver bug.

How do the proposal changes to buffer pointer ensure this bug is fixed?

I just don’t think it makes sense to close issues based on the assumption that a proposed feature will address the bug, without Microsoft actually testing to ensure the bug is fixed…

@natevm
Copy link
Contributor

natevm commented Sep 3, 2024

Note, I’m not saying DXC crashes. Rather, the generated SPIR-V seems to produce undefined behavior when used in a Vulkan pipeline.

@s-perron
Copy link
Collaborator

s-perron commented Sep 9, 2024

How do the proposal changes to buffer pointer ensure this bug is fixed?

Do you still see a crash? Have you tried since #6701 was merged? That could have been the problem. This same error will not happen again because we have a lit test for it.

I would also note that you cannot expect much to be done when all that was in this issue says is "I’ve noticed on AMD platforms that these raw buffer loads and stores cause their internal driver compiler to crash. Perhaps this has something to do with the layout rules?"

It is also in the middle of a larger discussion. Opening a specific issue for a bug instead of adding to an existing issue is better. This issue was about a feature request that was generally resolved. We have lit tests for the features we added.

@natevm
Copy link
Contributor

natevm commented Sep 10, 2024

It’s been a couple years now since that issue. I’ve moved over to Slang for HLSL->SPIR-V codegen, since that seemed to circumvent the issue.

We should re-test, yes. I don’t have my AMD card with me at the moment but I can try setting up a test bench again.

Generally, for “continuous testing” to have vendors address issues like this, I’ve found that if I can weasel in a use case in Sacha Willem’s Vulkan samples repository, a failing sample there seems to be caught by AMD with a bug report and then fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spirv Work related to SPIR-V
Projects
Archived in project
Development

No branches or pull requests

5 participants