[vulkan][asan] Fix Invalid Memory ops #41224
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 63782e9 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 31 times. |
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
| const bool hasBias = bias.has_value() && bias->defined(); | ||
| const bool vulkanBias = (*bias).is_vulkan(); | ||
| if (hasBias && vulkanBias) { | ||
| if (hasBias && (*bias).is_vulkan()) { |
There was a problem hiding this comment.
Alternatively you can write as
if (hasBias && bias->is_vulkan()) {
...
}
since the dereference operator is overloaded for c10::optional. Not that it really matters but just maybe a tiny bit more idiomatic.
| VBuffer buffer{sizeAligned}; | ||
| if (data.has_value()) { | ||
| buffer.copy_from_host_to_device((void*)*data, size); | ||
| buffer.copy_from_host_to_device((void*)*data, dataSize); |
There was a problem hiding this comment.
Any pointer type can implicity cast to void *. The cast should not be necessary.
| c10::optional<const float*> data, | ||
| const uint32_t size) { | ||
| const uint32_t dataSize, | ||
| const uint32_t bufferSize) { |
There was a problem hiding this comment.
I am not sure I understand why this function accepts bufferSize as a parameter as well when this function is the entity that ends up allocating the buffer itself a few lines below. Is it because you want to put more data inside this buffer later on and want to make sure you have enough storage?
There was a problem hiding this comment.
Yes, that is memory alignment. For example, if we want to bind in shader vec4[4], our buffer should be 4 * 4 * 4 (float size). While we might have only 14 floats.
[ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
Differential Revision: [D22754940](https://our.internmc.facebook.com/intern/diff/D22754940) [ghstack-poisoned]
|
@IvanKobzarev merged this pull request in 91d8729. |
Stack from ghstack:
Differential Revision: D22754940