Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix model loading on architectures where size_t is 32bits #825

Merged
merged 3 commits into from
Mar 19, 2021
Merged

Conversation

XapaJIaMnu
Copy link
Contributor

@XapaJIaMnu XapaJIaMnu commented Mar 4, 2021

Description

When porting marian to architectures of different width (x86_32/arm32/wasm) the binary file format would fail to load. The reason for that is that size_t on those architectures translates to uint32_t, whereas most commonly the binary models are built on x86_64. This pull request makes sure that when binary model is loaded or stored, all headers are of type uint64_t . This pull request would not change the existing functionality but would make porting to architectures of different width easier, as the programmer would not be facing cryptic errors when loading binary models.

  • I have tested the code manually
  • I have run regression tests
  • I have read and followed CONTRIBUTING.md
  • I have updated CHANGELOG.md

@emjotde
Copy link
Member

emjotde commented Mar 4, 2021

@snukky do we have any regression tests that use the *.bin models?
Might be time to add a small test that can do something with memory mapping, at least in a unit test.

@emjotde emjotde self-requested a review March 4, 2021 02:16
@emjotde
Copy link
Member

emjotde commented Mar 4, 2021

Let's merge the unit test one first PR: #826

@emjotde
Copy link
Member

emjotde commented Mar 4, 2021

Also, modify the changelog please.

@snukky
Copy link
Member

snukky commented Mar 4, 2021

do we have any regression tests that use the *.bin models?

@emjotde We have these that use .bin models, plus "wngt19" tests include versions for avx2 and avx512.

tests/decoder/intgemm/test_intgemm_16bit_avx2.sh
tests/decoder/intgemm/test_intgemm_16bit.sh
tests/decoder/intgemm/test_intgemm_16bit_sse2.sh
tests/decoder/intgemm/test_intgemm_8bit_avx2.sh
tests/decoder/intgemm/test_intgemm_8bit.sh
tests/decoder/intgemm/test_intgemm_8bit_ssse3.sh
tests/models/wngt19/test_model_base_fbgemm_packed16.sh
tests/models/wngt19/test_model_base_fbgemm_packed8.sh
tests/models/wnmt18/test_student_small_aan_intgemm16.sh
tests/models/wnmt18/test_student_small_aan_intgemm8.sh

@XapaJIaMnu
Copy link
Contributor Author

I updated the changelog. As roman said, the regression tests include plenty of binarizations and model loadings and since those past, I think it's fine.

BTW @emjotde did the big MPI fixes land?

@emjotde
Copy link
Member

emjotde commented Mar 10, 2021

RE: MPI, yes. that was the last sync.

@emjotde
Copy link
Member

emjotde commented Mar 10, 2021

I have to take a look what is going on with binary file loading in general on windows, then this PR should be ready to go in.
Something seems weird in #826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants