[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[ET-VK] Using a single GPU buffer for all tensor uniforms. - #7015

Merged
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head
Dec 5, 2024
Merged

[ET-VK] Using a single GPU buffer for all tensor uniforms.#7015
facebook-github-bot merged 9 commits into
gh/trivedivivek/14/basefrom
gh/trivedivivek/14/head

Conversation

@trviv

@trvivtrviv commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

This diff changes Tensor class to store all uniforms in a single uniform buffer.

Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.

Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.

Adding an offset input to Buffer::data function.

Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.

Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Nov 21, 2024

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7015

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit f3bc1e6 with merge base a04a87f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 21, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255514948
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 26, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255516791
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
trviv added a commit that referenced this pull request Nov 27, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 255668611
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
[ghstack-poisoned]
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D65841750

@facebook-github-bot
facebook-github-bot merged commit 3c1a2cf into gh/trivedivivek/14/baseDec 5, 2024
@facebook-github-bot
facebook-github-bot deleted the gh/trivedivivek/14/head branch December 5, 2024 20:18
kirklandsign pushed a commit that referenced this pull request Dec 5, 2024
Pull Request resolved: #7015
This diff changes Tensor class to store all uniforms in a single uniform buffer.
Entities stored in uniforms ie. size, stride, numel and logical limits are now stored in a single buffer and their offsets are stored as unsigned ints in Tensor class.
Other changes includes:
Adding a new ctor for ParamsBuffer class to allow allocation with size without data ptr.
Adding an offset input to Buffer::data function.
Adding an offset parameter to BufferBindInfo ctor, so additional offset can be supplied when binding a buffer.
ghstack-source-id: 256728325
@exported-using-ghexport
Differential Revision: [D65841750](https://our.internmc.facebook.com/intern/diff/D65841750/)
Co-authored-by: Vivek Trivedi <5340687+trivedivivek@users.noreply.github.com>
SS-JIA added a commit that referenced this pull request Jan 2, 2025
…ructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
[ghstack-poisoned]
SS-JIA added a commit that referenced this pull request Jan 2, 2025
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ructor (#7482)
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
…ed (#7483)
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
SS-JIA added a commit that referenced this pull request Jan 3, 2025
* [ET-VK][ez] Fix undefined behaviour in ambiguous `ParamsBuffer` constructor
## Context
I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.
The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.
To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.
Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)
ghstack-source-id: 260031108
Pull Request resolved: #7478
* [ET-VK] Create Pipeline layouts with push constant ranges when required
## Context
#7223 added the ability to use push constants in shaders. However, one thing the diff missed was not specifying that the compute pipeline layout needed to include a push constant upon creation. The Vulkan validation layers warns against this, and on certain GPUs such as the integrated Intel GPU on my windows laptop compute shaders will produce incorrect output.
This diff makes the change such that the compute pipeline layout will be created with a push constant block if necessary.
## Solution
Change the key of the pipeline layout cache to accept an additional push constant size field. The push constant size will be used to create the pipeline layout with a push constant block of the specified size.
Differential Revision: [D67770793](https://our.internmc.facebook.com/intern/diff/D67770793/)
ghstack-source-id: 260031109
Pull Request resolved: #7479
* [ET-VK] Fix metadata UBO VVL warnings
## Context
Recently #7015 was implemented so that all tensor metadata (e.g. sizes, strides) would be stored in a single UBO instead of with separate UBO objects. This helps with memory savings presumably due to defragmentation of memory allocations.
However, once the change was introduced, I noticed two new warnings produced by the Vulkan Validation Layer.
The first complains that the offset of a UBO descriptor is not a multiple of the `minUniformBufferOffsetAlignment` field reported by the physical device properties.
The second complains that the range of a UBO descriptor exceeds the offset + range of the underlying UBO object.
# Solution
To address the first one, instead of using `sizeof(utils::ivec4)` to determine the offset per metadata field, check the `minUniformBufferOffsetAlignment` field of reported by the device and use that instead.
The second warning arises because the logic in the constructor of `BufferBindInfo` had a mistake; instead of using the range of the underlying UBO object, it should use the range subtracted by the user specified offset.
Differential Revision: [D67770792](https://our.internmc.facebook.com/intern/diff/D67770792/)
ghstack-source-id: 260031110
Pull Request resolved: #7480
---------
Co-authored-by: Stephen Jia <ssjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.fb-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@trviv@facebook-github-bot@nathanaelsee@junpi3