Change vml.h to support sizes greater than 2**32 - 1 #17280
Conversation
cpuhrsch
changed the title
soumith
left a comment
soumith
left a comment
There was a problem hiding this comment.
what happens here if MKL_INT is int64_t? will it silently fail, or does it work fine?
If there's silent failure, can you static_assert
|
can we add a test? (how expensive is it?) |
|
@soumith - I think we'd need to worry about size_t, but for int64_t the max value should then cause it always stick to the first branch. There's a bunch of stuff left to do here, including benchmarks on the impact of writing these branches (which I'd be surprised if there are any). A static assert is still a good idea. In general, we should move all includes of mkl.h into a single location and reference our own header. There are some implicit settings one can do by defining variables before the include. This can then also include the static assert, which is a good way to check that PyTorch is only built with the lp64 version (as oppposed to ilp64). @gchanan - Since this is very specific to MKL VML and it's referenced in a single location, I'm not sure it's worth adding this to the tests we run for every built. |
We need nightlies! |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I ran some performance regression tests and the difference seems to be within the noise, so performance wise this should be fine. |
|
@pytorchbot rebase this please |
1 similar comment
|
@pytorchbot rebase this please |
cpuhrsch
changed the title
aten/src/ATen/cpu/vml.h
Outdated
There was a problem hiding this comment.
ship this, but we should add a global setting to allow building / linking against MKL_INT=int64_t as well, it just has to be non-default. Send a patch to do that in a follow-up PR.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
Approved via ghapprove D14154997
|
Thanks. Will merge soon. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Pull Request resolved: pytorch/pytorch#17280 Differential Revision: D14154997 Pulled By: cpuhrsch fbshipit-source-id: c19b15d18da59c9ee87e82765d3244d2a4ef6729
6 participants
No description provided.