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

[Hexagon] Remove "c" as aot_host_target tvm/contrib/hexagon/pytest_pl… #14997

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

kparzysz-quic
Copy link
Contributor

…ugin.py

After PR#14918 (changes to SplitHostDevice), LowerIntrin code generates fma, which then becomes the LLVM intrinsic fmuladd. Problem is, the code then goes to the C codegen, which knows nothing about LLVM intrinsics. The result is an abort at compile-time:

E     File ".../src/target/source/codegen_c.cc", line 611
E   TVMError: Unresolved call Op(tir.call_llvm_pure_intrin)

Why this didn't happen before is unclear at the moment, but using C codegen with Hexagon code doesn't sound like a winning combination. Hexagon-related code assumes LLVM codegen, and the above crash is the result of that.

…ugin.py

After PR#14918 (changes to SplitHostDevice), LowerIntrin code generates
fma, which then becomes the LLVM intrinsic fmuladd. Problem is, the code
then goes to the C codegen, which knows nothing about LLVM intrinsics.
The result is an abort at compile-time:
```
E     File ".../src/target/source/codegen_c.cc", line 611
E   TVMError: Unresolved call Op(tir.call_llvm_pure_intrin)
```

Why this didn't happen before is unclear at the moment, but using C
codegen with Hexagon code doesn't sound like a winning combination.
Hexagon-related code assumes LLVM codegen, and the above crash is the
result of that.
@tvm-bot
Copy link
Collaborator

tvm-bot commented May 31, 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

@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented May 31, 2023

cc: @Lunderberg
Maybe you know why it worked before without spending time on investigating... The current situation isn't wrong, but I'm wondering what we did before that was different.

@Lunderberg
Copy link
Contributor

Hmm. I think it might be related to this change in particular. Previously, the host function had its kTarget attribute removed altogether. Afterwards, the host function had its kTarget attribute updated to be the original functions target->GetHost(). Effectively, that would result in any per-function host annotations being replaced with the host passed to tvm.build, which defaults to LLVM.

That still wouldn't explain how the LLVM intrinsics were inserted in the first place, as LowerIntrin should use whatever the function annotation is at the point of the pass being applied, but it might be close to an answer.

@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented May 31, 2023

LowerIntrin inserted fma, which is a TIR builtin. Then, Hexagon code legalized it to an LLVM intrinsic.

Edit: Hexagon legalization assumes LLVM codegen, so it can unconditionally generate LLVM-specific code. This is what the PR comment refers to.

@Lunderberg
Copy link
Contributor

Ah, got it. I thought that the LLVM fma was being directly inserted by tir.transform.LowerIntrin.

Summing up, it sounds like Hexagon legalization assumes LLVM codegen, and so the "c" codegen shouldn't have been used in this test case. The test case worked previously, because SplitHostDevice removed the host information entirely, then defaulted to LLVM. The refactor in #14918 fixed that behavior in SplitHostDevice, which meant that it was no longer covering up the use of "c" runtime.

@quic-sanirudh quic-sanirudh merged commit 8543cec into apache:main Jun 1, 2023
mei-ye pushed a commit to mei-ye/tvm that referenced this pull request Jun 1, 2023
apache#14997)

[Hexagon] Remove "c" as aot_host_target tvm/contrib/hexagon/pytest_plugin.py

After PR#14918 (changes to SplitHostDevice), LowerIntrin code generates
fma, which then becomes the LLVM intrinsic fmuladd. Problem is, the code
then goes to the C codegen, which knows nothing about LLVM intrinsics.
The result is an abort at compile-time:
```
E     File ".../src/target/source/codegen_c.cc", line 611
E   TVMError: Unresolved call Op(tir.call_llvm_pure_intrin)
```

Why this didn't happen before is unclear at the moment, but using C
codegen with Hexagon code doesn't sound like a winning combination.
Hexagon-related code assumes LLVM codegen, and the above crash is the
result of that.
@kparzysz-quic kparzysz-quic deleted the hexagon-c branch June 1, 2023 11:57
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.

5 participants