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] Avoid call_extern() with incorrect argument count #15301

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, if device initialization is required, the AOT main function produced a call_extern() that included the device context as input. This commit updates the AOT main function to provide the device context only if the function being called accepts a device context as input.

If an extra device context argument is included at the call site, the C codegen would produce a function signature that includes the device context for the caller's compilation unit, but a signature without the device context for the callee's compilation unit. While this can compile and run in some cases, it is undefined behavior for the signature to vary between compilation units, and should be avoided.

This was initially discovered while debugging #14985, in which changes to the lowering flow resulted in the caller and callee being within the same compilation unit.

Prior to this commit, if device initialization is required, the AOT
main function produced a `call_extern()` that included the device
context as input.  This commit updates the AOT main function to
provide the device context only if the function being called accepts a
device context as input.

If an extra device context argument is included at the call site, the
C codegen would produce a function signature that includes the device
context for the caller's compilation unit, but a signature without the
device context for the callee's compilation unit.  While this can
compile and run in some cases, it is undefined behavior for the
signature to vary between compilation units, and should be avoided.

This was initially discovered while debugging
apache#14985, in which changes to the
lowering flow resulted in the caller and callee being within the same
compilation unit.
@Lunderberg Lunderberg requested a review from areusch July 12, 2023 14:46
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jul 12, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

1 similar comment
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jul 12, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@tqchen tqchen merged commit d81e880 into apache:main Jul 18, 2023
6 checks passed
junrushao pushed a commit to junrushao/tvm that referenced this pull request Jul 24, 2023
Prior to this commit, if device initialization is required, the AOT
main function produced a `call_extern()` that included the device
context as input.  This commit updates the AOT main function to
provide the device context only if the function being called accepts a
device context as input.

If an extra device context argument is included at the call site, the
C codegen would produce a function signature that includes the device
context for the caller's compilation unit, but a signature without the
device context for the callee's compilation unit.  While this can
compile and run in some cases, it is undefined behavior for the
signature to vary between compilation units, and should be avoided.

This was initially discovered while debugging
apache#14985, in which changes to the
lowering flow resulted in the caller and callee being within the same
compilation unit.
junrushao pushed a commit to junrushao/tvm that referenced this pull request Jul 27, 2023
Prior to this commit, if device initialization is required, the AOT
main function produced a `call_extern()` that included the device
context as input.  This commit updates the AOT main function to
provide the device context only if the function being called accepts a
device context as input.

If an extra device context argument is included at the call site, the
C codegen would produce a function signature that includes the device
context for the caller's compilation unit, but a signature without the
device context for the callee's compilation unit.  While this can
compile and run in some cases, it is undefined behavior for the
signature to vary between compilation units, and should be avoided.

This was initially discovered while debugging
apache#14985, in which changes to the
lowering flow resulted in the caller and callee being within the same
compilation unit.
junrushao pushed a commit to junrushao/tvm that referenced this pull request Jul 30, 2023
Prior to this commit, if device initialization is required, the AOT
main function produced a `call_extern()` that included the device
context as input.  This commit updates the AOT main function to
provide the device context only if the function being called accepts a
device context as input.

If an extra device context argument is included at the call site, the
C codegen would produce a function signature that includes the device
context for the caller's compilation unit, but a signature without the
device context for the callee's compilation unit.  While this can
compile and run in some cases, it is undefined behavior for the
signature to vary between compilation units, and should be avoided.

This was initially discovered while debugging
apache#14985, in which changes to the
lowering flow resulted in the caller and callee being within the same
compilation unit.
@Lunderberg Lunderberg deleted the aot_optional_device_context branch July 31, 2023 17:04
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