-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Highlight groups for the HTML Stmt file and tooltips to reveal types. #7887
Highlight groups for the HTML Stmt file and tooltips to reveal types. #7887
Conversation
@antonysigma Feel free to run the linter over the new JS. |
@mcourteaux For sure. Here you are: /home/antony/Projects/halide/src/irvisualizer/html_template_StmtToHTML.js
29:9 error 'matchedElements' is already defined no-redeclare
44:21 error 'highlightIdx' is already defined no-redeclare
65:5 error 'makeCodeVisible' is not defined no-undef
100:39 error 'event' is defined but never used no-unused-vars
156:18 error 'i' is already defined no-redeclare
166:13 error 'resizer' is already defined no-redeclare
167:18 error 'i' is already defined no-redeclare
199:51 error 'device_code_Tab' is not defined no-undef
✖ 8 problems (8 errors, 0 warnings) Feel free to https://github.com/antonysigma/halide/tree/javascript-scan-for-virus |
@antonysigma Amazing, thank you! I found this online "playground" for eslint. I managed to get the exact same report. That's super convenient to test in the future. Thanks for helping validate that this alternative is viable. You can use: https://eslint.org/play/ with the I'll improve upon the eslint issues, after which I think this is ready to merge. |
Cleaned up the code, all warnings are gone. This was actually useful! I retested the result, and all looks fine. Ready to merge. |
src/StmtToHTML.cpp
Outdated
@@ -1070,11 +1080,17 @@ class HTMLCodePrinter : public IRVisitor { | |||
context_stack_tags.push_back(tag); | |||
} | |||
|
|||
void print_opening_tag(const std::string &tag, const std::string &cls, std::string id) { | |||
stream << "<" << tag << " class='" << cls << "' id='" << id << "'>"; | |||
/* |
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.
Please delete commented-out code, or add a comment that explains why it is preserved.
src/StmtToHTML.cpp
Outdated
stream << body; | ||
print_closing_tag(tag); | ||
} | ||
|
||
void print_html_element(const std::string &tag, const std::string &cls, const std::string &body, std::string id) { | ||
print_opening_tag(tag, cls, id); | ||
/* |
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.
Same comment here.
src/StmtToHTML.cpp
Outdated
@@ -1542,10 +1574,15 @@ class HTMLCodePrinter : public IRVisitor { | |||
} | |||
|
|||
void visit(const Call *op) override { | |||
/* |
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.
More commented-out code here.
Oh sorry about those. I should make a habit of checking and reviewing the overall changes when I open a PR. |
Just to be clear, this is ready to merge. 🙂 |
Could you do a merge with main? That will hopefully clear up most of the CI failures. |
02984f8
to
b8f888e
Compare
Okay! Rebased on main. 😄 |
Thanks for merging! 🥳 I'm looking forward to hearing feedback on the HTML Stmt IR, as opposed to the plaintext Stmt IR files. I'm using them for a long time now, and I generally really like it. If there are obvious shortcomings, please let me know. I would love to see people start using this and prefer it. So if there's anything bothering/blocking you from using it, I'd be happy to hear it. Here is a demo file I hosted for quickly checking it out. Quick start: generator emit types are
|
This looks really cool and is indeed a big improvement over IR files!
I've pulled in the latest Halide changes into Google code base, so this should be available for internal use. I think we will need to write a short how-to on how to use it with Google build environment and after that we can advertise it in local Halide user group (actually, just last week someone asked me if there is an easy way to see a generated assembly for inner loops and it seems to fit the bill perfectly). |
Just as a note, it's not available through the Func or Pipeline C++ API, only through the Generators, which might not be a situation that applies to everyone. |
Unless things have changed since I was there, Google is 100% Generators |
@mcourteaux I am still catching up... Currently struggling with the emit option |
@antonysigma Sorry, this is the second time you report a bug like this. I fixed this one again. Hopefully all cases are covered. PR in #7914. |
…halide#7887) * Highlight groups for the HTML Stmt file and tooltips to reveal types. * Cleaned up JS using eslint. * Remove commented code.
While I was investigating the buffer index datatypes for #7884 myself, I added tooltips to reveal the datatype of the IR node in the Stmt file.
Another feature I wanted much, to analyze variables (especially when there is too much code to fit on the screen and you have to start scrolling, which makes the highlight-on-hover impractical) is to color matching variables by clicking them. There are 7 colors slots which will automatically recycle if you un-highlight a variable.
Also minor bugfix in the printing: Reinterpret casts were printed as normal casts before.