Support CPU Adam and Adagrad on Windows #1634
Conversation
lostmsu
requested review from
awan-10,
cli99,
conglongli,
eltonzheng,
jeffra,
minjiaz,
RezaYazdaniAminabadi,
samyam,
ShadenSmith and
tjruwase
as code owners
|
@jeffra @RezaYazdaniAminabadi the CI tests seem to be passing. Is there anything I need to do for this PR to be merged? It is blocking further work, as GitHub does not allow to submit more PRs on top of existing PRs. BTW, if you want, I can instead make one huge PR with many Windows-related changes, but they won't be as siloed as this one. |
| size_t H = layout.size(0); | ||
| size_t M = layout.size(1); | ||
| size_t N = layout.size(2); | ||
| int64_t H = layout.size(0); | ||
| int64_t M = layout.size(1); | ||
| int64_t N = layout.size(2); |
There was a problem hiding this comment.
Was this change required for CPU Adam+Adagrad support on windows?
There was a problem hiding this comment.
Oh I see, you're right here, this is fixing a bug I think.
https://pytorch.org/cppdocs/api/classat_1_1_tensor.html#_CPPv4NK2at6Tensor4sizeEN2at7DimnameE
There was a problem hiding this comment.
I, honestly, forgot what the compile error was, but torch's C++ size explicitly returns int64_t.
There was a problem hiding this comment.
Can we use uint64_t instead, because if I remember correctly size_t is unsigned long int?
Ensures, that CPU Adagrad and Adam can be build with MSVC++
Unit tests for them appear to pass on my machine, but I am hoping to add a CI build in the future too to ensure they don't break.