-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[cc] Compose ActionRecorder outputs into a single C file for Emscripten #1629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1629 +/- ##
==========================================
- Coverage 45.00% 44.29% -0.71%
==========================================
Files 40 41 +1
Lines 5739 5858 +119
Branches 1002 1017 +15
==========================================
+ Hits 2583 2595 +12
- Misses 3002 3116 +114
+ Partials 154 147 -7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Just some minor issues that need a little clarification.
taichi/program/kernel.cpp
Outdated
{ActionArg("kernel_name", name), ActionArg("arg_id", i), | ||
ActionArg("address", fmt::format("0x{:x}", ptr)), | ||
ActionArg("array_size_in_bytes", (int64)size)}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still people using information like set_kernel_arg_int64
. Can we keep these for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it doesn't even include dtype
... They are just spams in our compile-only no-launch action parser, both in CC and [数据删除] in my pratice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there are still people depending on this. I don't think we should remove them immediately. Maybe a better solution is to mark them as a different type of message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the feature is purposely undocumented
and used by only you and me and the [数据删除]? Also did you confirmed this doesn't harm performance? IMO we'd remove all the run-time action output iapr, since only compile-time kernel information are required for reconstructing a Taichi program outside, e.g., this PR, did that easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, much better now. I still suggest keeping the kernel argument information.
taichi/program/kernel.cpp
Outdated
{ActionArg("kernel_name", name), ActionArg("arg_id", i), | ||
ActionArg("address", fmt::format("0x{:x}", ptr)), | ||
ActionArg("array_size_in_bytes", (int64)size)}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there are still people depending on this. I don't think we should remove them immediately. Maybe a better solution is to mark them as a different type of message.
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you so much!
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
Related issue = #
[Click here for the format server]
Usage e.g.:
Then write a mpm88.html that includes taichi.js and mpm88.js and the following inline scripts:
See https://github.com/taichi-dev/taichi.js for client, please run
python server.py
to set up a simple http server, it won't work infile:///
.Btw, I forget to grant myself with admin permission while transfer... LOL, @yuanming-hu Would you mind restore me with admin and make the taichi.js repo to be public so that people could share it?