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

[Web] Add TVMArgBool to ArgTypeCode #17251

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

CharlieFRuan
Copy link
Contributor

In a recent PR #16183, TVMArgTypeCode introduced TVMArgBool. This PR is needed for the web runtime when a TVM packed function returns a bool.

@@ -2473,6 +2473,7 @@ export class Instance implements Disposable {
switch (tcode) {
case ArgTypeCode.Int:
case ArgTypeCode.UInt:
case ArgTypeCode.TVMArgBool:
return this.memory.loadI64(rvaluePtr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @tqchen, would loadI64() be the right function to use for bool?

Copy link
Member

Choose a reason for hiding this comment

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

this would need to wait until #17240, but once the new bool type lands yes this would be the case

@tqchen
Copy link
Member

tqchen commented Aug 7, 2024

temp block this in light of #16183 (comment)

@Lunderberg
Copy link
Contributor

#17240 has now landed, so this PR should be able to be revisited.

@tqchen tqchen merged commit 1518008 into apache:main Aug 23, 2024
16 checks passed
CharlieFRuan added a commit to mlc-ai/web-llm that referenced this pull request Aug 23, 2024
### Change
- #555

### TVMjs
- Updated to current head:
apache/tvm@1518008
  - Main change is apache/tvm#17251
- This is needed for WASMs compiled after
apache/tvm#17257 is merged (e.g. Phi-3.5). TVM
global functions that returns bool need this PR to run correctly (e.g.
`AcceptToken()` in BNFGrammar) in runtime.
- However, these are backward compatible to WASMs compiled prior to this
PR. Tested with Phi-3 (old WASM) running grammar.
jzhao62 pushed a commit to jzhao62/web-llm that referenced this pull request Dec 8, 2024
### Change
- mlc-ai#555

### TVMjs
- Updated to current head:
apache/tvm@1518008
  - Main change is apache/tvm#17251
- This is needed for WASMs compiled after
apache/tvm#17257 is merged (e.g. Phi-3.5). TVM
global functions that returns bool need this PR to run correctly (e.g.
`AcceptToken()` in BNFGrammar) in runtime.
- However, these are backward compatible to WASMs compiled prior to this
PR. Tested with Phi-3 (old WASM) running grammar.
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