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

[AOT] fix cross platform issue: using uint64_t to replace size_t. #2626

Merged
merged 5 commits into from Apr 4, 2019

Conversation

Projects
None yet
5 participants
@xizhizhang
Copy link
Contributor

xizhizhang commented Mar 30, 2019

Description: Ahead-of-time Compilation cross platform issue: build on 64-bit OS and run on 32-bit OS.
Testing: x86_64 and armv7 platform.
Documentation: modify AOT.md

Fixes #2592

xizhizhang added some commits Mar 30, 2019

fix for example build: ResNet50Bundle.
    build environment:

        ubuntu 18.04

    error message:

        ...
        error: use of undeclared identifier 'strlen'
        error: use of undeclared identifier 'memset'
        ...
fix cross platform issue: using uint64_t to replace size_t.
more detail information is at issue #2592
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Mar 30, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Mar 30, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jfix71

jfix71 approved these changes Mar 30, 2019

Copy link
Contributor

jfix71 left a comment

Thanks for the fix, just one question but LGTM!

Show resolved Hide resolved examples/bundles/resnet50/main.cpp
// size_t offset;
// size_t size;
// uint64_t offset;
// uint64_t size;
// char kind;
// };
auto *charTy = llvm::Type::getInt8Ty(irgen_->getLLVMContext());
auto *sizeTTy =

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 1, 2019

Contributor

The name sizeTTy doesn't make a lot of sense if we use uint64_t now

// Get the integer type having the same size in bits as size_t.
auto *SizeTType = irgen_->getBuilder().getIntNTy(sizeof(size_t) * 8);
// Get the integer type having the same size in bits as uint64_t.
auto *SizeTType = irgen_->getBuilder().getIntNTy(sizeof(uint64_t) * 8);

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 1, 2019

Contributor

The name SizeTTy doesn't make a lot of sense if we use uint64_t now. Also the name should start with a lower-case.

This comment has been minimized.

Copy link
@petecoup

petecoup Apr 2, 2019

Contributor

Does it make more sense to use the newly introduced LLVMIRGen::getTargetSizeTWidth?

This comment has been minimized.

Copy link
@opti-mix

opti-mix Apr 2, 2019

Contributor

@petecoup If we decide to go for target-specific size_t usage, we would definitely use LLVMIRGen::getTargetSizeTWidth.

We are still trying to decide if it is better to use fixed-size fields instead, so that the BundleConfig has the same binary layout on all platforms and can be easily read e.g. by tools, helper functions, etc without thinking about any target-specific aspects.

This comment has been minimized.

Copy link
@petecoup

petecoup Apr 2, 2019

Contributor

Sorry, I somehow missed the later comments...this makes sense!

@opti-mix

This comment has been minimized.

Copy link
Contributor

opti-mix commented Apr 1, 2019

@jfix71 @xizhizhang It was me who suggested to use uint64_t, but looking at this PR now and seeing how many changes it introduces I ask myself if it is a right thing to do?

My original intent was to make the BundleConfig struct use only fixed with integer types, so that this config always has the same size and layout and can be read on any platform and by any tool. And I still like it, sort of.

If we'd go for a target-platform specific size_t, we'll have less source code changes, but we won't be able to read the BundleConfig on a different target or with some kind of target-independent tool...

So, which of these two approaches is more future proof?

@jfix71 WDYT?

@jfix71

This comment has been minimized.

Copy link
Contributor

jfix71 commented Apr 1, 2019

@opti-mix I think I'd still prefer using uint64_t. And actually looking at the PR again, do we need to do so many size_t -> uint64_t changes? @xizhizhang How did you decide what to change to uint64_t, was it just a replace all size_t with uint64_t? E.g. You moved imageDataSizeInBytes to uint64_t, but it's used for memcpy() which takes size_t. Perhaps we should still use size_t in many places and just verify that the uint64_t in BundleConfig do not overflow size_t?

@xizhizhang

This comment has been minimized.

Copy link
Contributor Author

xizhizhang commented Apr 2, 2019

@opti-mix I think I'd still prefer using uint64_t. And actually looking at the PR again, do we need to do so many size_t -> uint64_t changes? @xizhizhang How did you decide what to change to uint64_t, was it just a replace all size_t with uint64_t? E.g. You moved imageDataSizeInBytes to uint64_t, but it's used for memcpy() which takes size_t. Perhaps we should still use size_t in many places and just verify that the uint64_t in BundleConfig do not overflow size_t?

I am sorry I just replace all size_t with uint64_t. You are right, we need not so many uint64_t.

I replaced them because I did not want to see any size_t. As an embedded C programmer, I have meet a lot of similar cross platform problem because of size_t / int / long (something looks designed for cross platform, but actually confuse and raise bugs).

@opti-mix Yeah, I prefer a target-specific size, but not size_t . Because, uint64_t in BundleConfig makes application running right, but, there is no need 64-bit member always(E.g. alignment / numSymbols). Because 64-bit member always need several instructions to finish basic calculation (E.g. add/sub/mul/div) on 32-bit CPU.

In my opinion, "all size_t" -> "all uint64_t" -> "every value have it's best type" helps me develop application and debug.
on the other hand, "all size_t" -> "some size_t and some uint64_t" is another way, maybe it is more orthodox method, and better to keep code stable.

Any way, I would like to follow this project line of thinking, let me known how should I modify this pull requests.

@jfix71
Copy link
Contributor

jfix71 left a comment

@xizhizhang I would say to basically change only BundleConfig to uint64_t, and keep as much otherwise in size_t, perhaps adding checks for overflowing size_t as necessary. And these are just examples for bundles, after all -- if you prefer all fixed width sizes instead then you could always create your own example.

Please address @opti-mix's comments as well as changing back as many uint64_t to size_t as possible, and this should be good to go. Thanks for your contributions!

xizhizhang added some commits Apr 3, 2019

@xizhizhang

This comment has been minimized.

Copy link
Contributor Author

xizhizhang commented Apr 3, 2019

@xizhizhang I would say to basically change only BundleConfig to uint64_t, and keep as much otherwise in size_t, perhaps adding checks for overflowing size_t as necessary. And these are just examples for bundles, after all -- if you prefer all fixed width sizes instead then you could always create your own example.

Please address @opti-mix's comments as well as changing back as many uint64_t to size_t as possible, and this should be good to go. Thanks for your contributions!

Thanks, but I keep uint64_t in SymbolTableEntry, it looks have similar problem with BundleConfig .

@opti-mix I renamed SizeTTy to uint64TTy

@opti-mix

This comment has been minimized.

Copy link
Contributor

opti-mix commented Apr 3, 2019

@xizhizhang Looks good!

But our CI complains about bad formatting:

diff --git a/lib/LLVMIRCodeGen/BundleSaver.cpp b/lib/LLVMIRCodeGen/BundleSaver.cpp
index d3d95da..098341d 100644
--- a/lib/LLVMIRCodeGen/BundleSaver.cpp
+++ b/lib/LLVMIRCodeGen/BundleSaver.cpp
@@ -89,9 +89,9 @@ void BundleSaver::emitSymbolTable() {
   auto *charTy = llvm::Type::getInt8Ty(irgen_->getLLVMContext());
   auto *uint64TTy =
       llvm::Type::getIntNTy(irgen_->getLLVMContext(), sizeof(uint64_t) * 8);
-  auto symbolTableEntryTy =
-      llvm::StructType::get(irgen_->getLLVMContext(),
-                            {charTy->getPointerTo(), uint64TTy, uint64TTy, charTy});
+  auto symbolTableEntryTy = llvm::StructType::get(
+      irgen_->getLLVMContext(),
+      {charTy->getPointerTo(), uint64TTy, uint64TTy, charTy});

Could you run utils/format.sh on your sources?

@xizhizhang

This comment has been minimized.

Copy link
Contributor Author

xizhizhang commented Apr 4, 2019

@xizhizhang Looks good!

But our CI complains about bad formatting:

diff --git a/lib/LLVMIRCodeGen/BundleSaver.cpp b/lib/LLVMIRCodeGen/BundleSaver.cpp
index d3d95da..098341d 100644
--- a/lib/LLVMIRCodeGen/BundleSaver.cpp
+++ b/lib/LLVMIRCodeGen/BundleSaver.cpp
@@ -89,9 +89,9 @@ void BundleSaver::emitSymbolTable() {
   auto *charTy = llvm::Type::getInt8Ty(irgen_->getLLVMContext());
   auto *uint64TTy =
       llvm::Type::getIntNTy(irgen_->getLLVMContext(), sizeof(uint64_t) * 8);
-  auto symbolTableEntryTy =
-      llvm::StructType::get(irgen_->getLLVMContext(),
-                            {charTy->getPointerTo(), uint64TTy, uint64TTy, charTy});
+  auto symbolTableEntryTy = llvm::StructType::get(
+      irgen_->getLLVMContext(),
+      {charTy->getPointerTo(), uint64TTy, uint64TTy, charTy});

Could you run utils/format.sh on your sources?

Thanks. I have forgotten it.

@opti-mix
Copy link
Contributor

opti-mix left a comment

Looking good!

@xizhizhang Thanks again for your contribution!

@opti-mix

This comment has been minimized.

Copy link
Contributor

opti-mix commented Apr 4, 2019

@jfix71 Could you put your stamp? And let's merge it!

@jfix71

jfix71 approved these changes Apr 4, 2019

Copy link
Contributor

jfix71 left a comment

LGTM, thanks @xizhizhang!

@jfix71 jfix71 merged commit d4fbe12 into pytorch:master Apr 4, 2019

6 checks passed

ci/circleci: ASAN Your tests passed on CircleCI!
Details
ci/circleci: DEBUG Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_WITH_EXPENSIVE_TESTS Your tests passed on CircleCI!
Details
ci/circleci: SHARED Your tests passed on CircleCI!
Details
ci/circleci: TSAN Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.