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 executor for different compilers #8006

Merged
merged 3 commits into from
May 10, 2021
Merged

Fix executor for different compilers #8006

merged 3 commits into from
May 10, 2021

Conversation

rijulg
Copy link
Contributor

@rijulg rijulg commented May 7, 2021

At the moment compiling this file throws multiple errors with C++ compilers, this change proposes to fix them.

  1. tvm_model_t->run_func of type TVMBackedPackedFunc returns an int at the moment which is different from the signature of this function tvm_runtime_run, implicit casting is not favorable in many compile chains and throws errors.
  2. The index of iterators were of type int while that of model->num_input_tensors and model->num_output_tensors were of type uint32_t, this type difference again throws errors in many toolchains, and can potentially cause incorrect calculations.
  3. C Style struct initialization of tensors with (DLTensor){...} is not supported in many C++ toolchains and throws “non-trivial designated initializers not supported” error. Explicitly setting values should work in all cases even though it looks a little less nice.

At the moment compiling this file throws multiple errors with C++ compilers, this change proposes to fix them.
1. `tvm_model_t->run_func` of type `TVMBackedPackedFunc` returns an int at the moment which is different from the signature of this function `tvm_runtime_run`, implicit casting is not favorable in many compile chains and throws errors.
2. The index of iterators were of type `int` while that of `model->num_input_tensors` and `model->num_output_tensors` were of type `uint32_t`, this type difference again throws errors in many toolchains, and can potentially cause incorrect calculations.
3. C Style struct initialization of tensors with `(DLTensor){...}` is not supported in many C++ toolchains and throws “non-trivial designated initializers not supported” error. Explicitly setting values should work in all cases even though it looks a little less nice.
@leandron
Copy link
Contributor

leandron commented May 7, 2021

cc @giuseros @Mousius

@jcf94
Copy link
Contributor

jcf94 commented May 8, 2021

Looks like this is a .c file, and I'm guessing is this designed to be compiled by a C compiler?

@rijulg
Copy link
Contributor Author

rijulg commented May 8, 2021

Looks like this is a .c file, and I'm guessing is this designed to be compiled by a C compiler?

Yes, it is probably designed to be compiled with a C compiler, but problems 1 and 2 should be encountered with any compilation process that prohibits implicit typecasting. Additionally the file can readily be used with C++ compiler, so it might be worth changing the structure initialisation to work with C++ compilers.

@jcf94
Copy link
Contributor

jcf94 commented May 8, 2021

Looks like this is a .c file, and I'm guessing is this designed to be compiled by a C compiler?

Yes, it is probably designed to be compiled with a C compiler, but problems 1 and 2 should be encountered with any compilation process that prohibits implicit typecasting. Additionally the file can readily be used with C++ compiler, so it might be worth changing the structure initialisation to work with C++ compilers.

Yeah, I agree. Besides, we should better use size_t instead of the uint32_t.

Copy link
Contributor

@jcf94 jcf94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure this code can work well in gcc and your own compiler after these modifications.
The other parts looks good to me.

src/runtime/crt/aot_executor/aot_executor.c Outdated Show resolved Hide resolved
@rijulg
Copy link
Contributor Author

rijulg commented May 9, 2021

@jcf94 Changed the type and tested with gcc as well as g++

Copy link
Contributor

@jcf94 jcf94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @rijulg

@jcf94 jcf94 merged commit 12b1417 into apache:main May 10, 2021
@rijulg rijulg deleted the patch-1 branch May 10, 2021 06:17
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request May 19, 2021
* Fix executor for different compilers

At the moment compiling this file throws multiple errors with C++ compilers, this change proposes to fix them.
1. `tvm_model_t->run_func` of type `TVMBackedPackedFunc` returns an int at the moment which is different from the signature of this function `tvm_runtime_run`, implicit casting is not favorable in many compile chains and throws errors.
2. The index of iterators were of type `int` while that of `model->num_input_tensors` and `model->num_output_tensors` were of type `uint32_t`, this type difference again throws errors in many toolchains, and can potentially cause incorrect calculations.
3. C Style struct initialization of tensors with `(DLTensor){...}` is not supported in many C++ toolchains and throws “non-trivial designated initializers not supported” error. Explicitly setting values should work in all cases even though it looks a little less nice.

* changing type to size_t

* fix format for clang
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
* Fix executor for different compilers

At the moment compiling this file throws multiple errors with C++ compilers, this change proposes to fix them.
1. `tvm_model_t->run_func` of type `TVMBackedPackedFunc` returns an int at the moment which is different from the signature of this function `tvm_runtime_run`, implicit casting is not favorable in many compile chains and throws errors.
2. The index of iterators were of type `int` while that of `model->num_input_tensors` and `model->num_output_tensors` were of type `uint32_t`, this type difference again throws errors in many toolchains, and can potentially cause incorrect calculations.
3. C Style struct initialization of tensors with `(DLTensor){...}` is not supported in many C++ toolchains and throws “non-trivial designated initializers not supported” error. Explicitly setting values should work in all cases even though it looks a little less nice.

* changing type to size_t

* fix format for clang
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
* Fix executor for different compilers

At the moment compiling this file throws multiple errors with C++ compilers, this change proposes to fix them.
1. `tvm_model_t->run_func` of type `TVMBackedPackedFunc` returns an int at the moment which is different from the signature of this function `tvm_runtime_run`, implicit casting is not favorable in many compile chains and throws errors.
2. The index of iterators were of type `int` while that of `model->num_input_tensors` and `model->num_output_tensors` were of type `uint32_t`, this type difference again throws errors in many toolchains, and can potentially cause incorrect calculations.
3. C Style struct initialization of tensors with `(DLTensor){...}` is not supported in many C++ toolchains and throws “non-trivial designated initializers not supported” error. Explicitly setting values should work in all cases even though it looks a little less nice.

* changing type to size_t

* fix format for clang
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