-
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
HTML Stmt IR with conceptual code and device code. #7843
Conversation
…nto pseudo-stmt-ir
@steven-johnson I believe you work at Google? Does anybody you know use the VizTree stuff? I highly doubt it. @abadams You work at Adobe I think? Does anybody you know use the VizTree stuff? Do you have opinions on whether to keep the VizTree thing or not? It looks pretty, but I don't see any use for it. |
I'm not aware of anyone using the VizTree stuff here at Adobe. You're the person most invested in the stmt html output (though that may change with the functionality you're adding!), so I defer to you entirely on what should and should not be included. |
Oh wow? I'm surprised. Nobody uses the generated HTML? How do people review the schedule they built for a certain pipeline? You can't just go based off the Almost 100% of the time, my workflow is iterating between reviewing the stmt code looking for inefficiencies, and improving the schedule, and confirming the improvement by benchmarks. |
Just personal experience: I 100% agree that it's far better to iterate on the schedule by looking for inefficiencies in the generated statement is. That being said, I pretty much always just use a plain statement in terminal/text editor instead of the HTML one. Previously, I felt that overhead of switching to browser to look at the statement was too high for me. I should try a new visualizer though, it looks pretty interesting. I don't remember if advertised new visualizer for internal users at Google, but we certainly can once it stabilizes (maybe it's already stable, but there were a lot of changes recently, so I am not sure). |
I also use the text-based stmt and assembly. |
Okay, then I would just discard this feature, and maybe park it in a stale branch, if anybody would like to revive it at one point. I'll finalize this PR tonight or tomorrow. |
While I work 95% of the time in the terminal for almost all I do, I think I'd prefer the HTML one for folding, syntax highlighting, and variable cross-matching, parenthesis highlighting, and the jump-to-assembly buttons. Do you have any syntax highlighting when investigating in the terminal? A vim syntax file? Some rainbow-parenthesis plugins? Or just "black and white"? |
Just black and white. I think with the ability to hop between asm and stmt easily I may start using the html version though. |
The HTML version would probably be preferable now; I think it's mainly inertia that keeps the core devs on the .stmt file, with a smattering of a slightly less convenient workflow in some cases (e.g.: I can open the .stmt file in an editor that will just auto-refresh every time I re-run the Generator, whereas .html requires a manual refresh. Trivial? Yes, but these are the things that add up to inertia.) |
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.
Many thanks @mcourteaux for the major revamp! I haven't tested the code yet. In the meantime, I quickly ran eslint
liner tool to flush out Javascript errors. I am attaching the logs below for your reference.
Regards,
Antony
var matchingElements = document.querySelectorAll(selector + ' .matched[id^="' + idPrefix + '-"]'); | ||
|
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.
Shall we format the code with clang-format
? The coding style configuration is already defined in the project root folder as .clang-format
. The tool can also recognize Javascript.
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.
Nice! Great suggestion!
line = lineSpans[lno - 1] | ||
line.scrollIntoView({ | ||
behavior: "smooth", |
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.
My linter catches some errors; all are minute. I am attaching the log here for your reference.
/home/antony/Projects/halide/src/irvisualizer/html_template_StmtToHTML.js
33:10 error 'scrollToCode' is defined but never used no-unused-vars
36:5 error 'makeCodeVisible' is not defined no-undef
57:13 error Unexpected console statement no-console
61:17 error 'rect' is defined but never used no-unused-vars
71:39 error 'event' is defined but never used no-unused-vars
85:9 error Unexpected console statement no-console
119:10 error 'collapse_tab' is defined but never used no-unused-vars
129:10 error 'scrollToHostAsm' is defined but never used no-unused-vars
137:10 error 'scrollToDeviceCode' is defined but never used no-unused-vars
140:5 error 'line' is not defined no-undef
141:5 error 'line' is not defined no-undef
142:28 error Unexpected trailing comma comma-dangle
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.
This is the content of my .eslint.js
config:
module.exports = {
env: {
es6: true,
node: true,
browser: true
},
extends: [
'eslint:recommended',
],
parserOptions: {
// Enable JSX support
ecmaFeatures: { jsx: true },
ecmaVersion: 2018,
sourceType: 'module'
},
//parser: 'babel-eslint',
rules: {
// Add any additional rules here
'react/prop-types': 0
}
}
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.
Thank you for these! I will look into it. Is there ways to suppress some of these? Some of these "unused functions" are of course not unused, as the HTML is sprinkled with onclick="scrollToHostAsm()"
for example.
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.
Yeah, I found that this should work:
function scrollToDeviceCode(lno) { // eslint-disable-line no-unused-vars
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 haven't looked at running eslint myself, but if you'd like to give it another go, feel free to!
For sure. I am linting the StmtToHTML.js
only.
halide/src/irvisualizer/html_template_StmtToHTML.js
34:5 error 'makeCodeVisible' is not defined no-undef
69:39 error 'event' is defined but never used no-unused-vars
146:51 error 'device_code_Tab' is not defined no-undef
@mcourteaux Ping me again if you want a sanity check for legacy web browsers:
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 don't know how to interpret the screenshot of this legacy browser check. There are a bunch of numbers, but I don't know what they all indicate. Please keep in mind that this Halide pipeline visualizer HTML is a tool for developers who work on a desktop. The browsers we care about are all desktop based: Firefox, Chrome, Edge, Safari, Opera, would probably cover most of them.
…byte from device code buffers.
@antonysigma I attempted to fix your immediate feedback. I haven't looked at running eslint myself, but if you'd like to give it another go, feel free to! I addressed them I believe. |
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 have a few minor UX related questions. Please read the inline comments.
Regards,
Antony
Update: I like the green arrows to hop between the NVPTX panel and the Stmt panel. Not all offloaded GPU code has the green arrow, but yeah it exceed all my expectations. Attaching a demo HTML here to reach a broader audience, including but not limited to @abadams and @vksnk .
src/StmtToViz.cpp
Outdated
stream << " <button class='collapse-left' onclick='collapseTab(" << num << ")' title='Collapse pane on the left'></span>\n"; | ||
stream << " </button>\n"; | ||
stream << " </div>\n"; | ||
stream << " <div>\n"; | ||
stream << " <button class='collapse-right' onclick='collapseTab(" << (num + 1) << ")' title='Collapse pane on the right'></span>\n"; |
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.
A minor UX issue: Once I clicked the top button to collapse the stmt tab, the bottom button clears out the assembly tab.
Both buttons change to a right-handed arrow. The arrows are too tiny to tell which is which. I supposed it is a feature, not a bug?
P.S. it is a pity we have to reinvent the wheels here just to avoid 3rd party libraries. Something like this split.js should have saved us the headache.
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.
Yeah, I was trying to help distinguish this by shading the button. My main concern is screen estate. The green seperator is already quite thick, and wasting screen estate. For now, I decided to not waste more space just to be able to put a bigger arrow icon. I never really looked at the icons anyway. I just picked a few unicode arrows, slammed them on, and realized I was looking at the color of the button instead of the icon.
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.
A minor UX issue: Once I clicked the top button to collapse the stmt tab, the bottom button clears out the assembly tab.
Wait, that's exactly what it's supposed to do right? The button toggles the collapsedness of the tab left or right of it. Top button controls the tab on the left. Bottom button controls the tab on the right. You just click them to toggle whether they are collapsed or not.
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.
Marked as resolved.
That demo file doesn't work, as the dependent files (the Lines 20 to 25 in de1970f
|
src/IRPrinter.cpp
Outdated
@@ -1085,10 +1085,35 @@ void IRPrinter::visit(const Shuffle *op) { | |||
} | |||
|
|||
void IRPrinter::visit(const VectorReduce *op) { | |||
std::string op_str; |
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.
This should use the existing operator<< for vector reduce op types (with the implementation changed to snake case if you like)
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 see you changed the implementation of operator<< to use snake case, so I think the changes to this function can be mostly reverted
@@ -710,7 +710,7 @@ WEAK int halide_hexagon_dma_power_mode_voting(void *user_context, halide_hexagon | |||
val = ~PW_SVS; | |||
break; | |||
default: | |||
error(user_context) << "Hexagon: halide_hexagon_dma_power_voting power mode (", cornercase, ") not found\n"; | |||
error(user_context) << "Hexagon: halide_hexagon_dma_power_voting power mode (" << cornercase << ") not found\n"; |
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.
Nice catch. How did you find it?
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.
Good question. I only realize now that you'd be very interested in catching this automatically. One of the compilers I used over the last weeks was complaining about it. Could have been clamg on macOS.
Generally speaking, looks good! I just had some minor comments. |
For unrelated reasons, I'm not going to have the bandwidth to do the review and coordination on this for a while -- looks like @abadams is taking over (thanks!) |
Thank you very much for the review, @abadams! I'm currently on holidays. I'll address these comments properly when I'm back. The terms 2GL and 3GL were introduced into this discussion by @antonysigma in the original issue. The wiki page describes it quite well. I am willing to change it of course, I also didn't know the terms. |
@steven-johnson If the build errors are not my fault, then @abadams this is ready. |
One more super-minor comment where I think you missed one of the suggested changes. Also if you do a merge with main it should unbreak the builds. |
Oh well caught! Thanks. Merged with main as well. Let's see :) |
Doing some final experimenting with the colorscheme of the cost model. Hold off on merging for a few hours. |
Only failure is unrelated and will be fixed by #7868. Merging. |
* WIP: Conceptual Stmt IR and HTML cleanup. * WIP: Lots of progress on Stmt HTML. Cleanup almost complete. * Support scrolling to device code. * Resizing works decent enough for me. Fix cost-model allocate block costs. * Print better vector_reduce calls. * Optionally enable VizTree through an env variable. * Fix the device code tab for non-PTX. * StmtHTML: Tabs renamed to panes. Fix linter warnings. Cut trailing 0 byte from device code buffers. * Fix clang-format. * Fixed typos and copy paste error. * Fix HL_EXTRA_OUTPUTS behaviour to respect the defaults. * Nuked VizTree * Finalize StmtToViz nuke and rename StmtToHTML. * Improved HTML correctness by running output through an online validator. Quite some bugs fixed. * Cost model visualization improvement. Fix button not being allowed in the checkbox/label combination. * Fix collapsing being triggered by jump-to-xxx buttons. * How did this work? * Process Andrew's feedback. * Process Andrew's feedback. * Process Andrew's feedback, part 3. * Improve color palette. Few minor improvements. * Clang-format...
Features and functionality in this PR
Implemented the Generator options to emit
conceptual_stmt
andconceptual_stmt_html
:The Conceptual Stmt HTML is particularly useful when working on GPU schedules, as you get to see the Stmt code just before offloading the GPU-related loops to LLVM and getting device code back. This addresses stmt and stmt_html output are too low level #7519.
Added "Jump to Device code" buttons (above in green).
Added a Device Code pane to the page, which replaces the inline buffer in the Stmt IR. Buffers' content are no longer included in the Stmt IR.
Collapsing the subtrees does no longer recompute the costs (movement and computation) of the nodes. Data moving and computation costs are always shown both for the line itself, and for the whole subtree (left is for a the subtree (aka: including children), and right is the line itself):
Note that I added one extra color in the coloring scheme for zero-cost nodes: transparent.
Added some shading of GPU-related for-loops and functions. I tried to make it subtle but clear when you actually want to see the background shading.
Added
device_code
emit option for Generators, which will produce a.device_code
file. This currently only works if the device code is generated byOffloadGPULoops
. I'm not familiar with Hexagon.HTML-CSS-JS specific changes:
In general, I would finally consider the clean (no-VizTree) HTML Stmt IR as sane. I'm not a front-end developer, but I know a few things of keeping things simple.
Split the code (HTML/CSS/JS) from the VizIR stuff into the parts that belong to the clean Stmt IR HTML and the parts that belong to the Viz Tree stuff. These are now separate files and are included in the generated result depending on whether the VizTree is enabled.Added the possibility to setHL_HTML_VIZTREE=1
as an environment variable which will enable the VizTree generation.:checked
checkbox inputs.Relevant other changes:
vector_reduce(op, data)
now asvector_reduce_op(data)
. This turnsvector_reduce(0, data)
intovector_reduce_add(data)
, which is better to read. The reason this VectorReduce::Operator printed as an integer is because there is no such thing as aIRVisitor::visit(VectorReduce::Operator)
.HL_EXTRA_OUTPUTS
would cause the default emit-options to be not used when originally the Generator was invoked without any explicit emit flags. No emit flags means to generate "static_library,cpp_registration,c_header", but once you specifyHL_EXTRA_OUTPUTS
that would cause the logic skip installing the default ones. @abadams @steven-johnson Please verify that this behavior is fine. I was using thisHL_EXTRA_OUTPUTS=stmt_html,conceptual_stmt_html make
in some of theapps/
, but would fail because now thec_header
and thestatic_library
options were no longer implied.PR status
Currently, the VizTree stuff is not revived yet, as my focus was to clean up the messy HTML and JavaScript code for the clean Stmt IR HTML. I didn't throw out anything fundamental yet from the VizTree. Opinions on this are welcome! Of course it will be easier to delete the remaining stuff than to revive it, and honestly, I would rather delete it than revive as I personally have absolutely no use for it.UPDATE: I nuked the VizTree code. This brings the PR to be ready for review I believe.
Tested the result (of the clean Stmt IR HTML) on Chromium and Firefox.
Tested with:
Only PTX code right now is syntax highlighted, and the other should just be pasted in without any coloring. Syntax highlighting for other languages welcome!
Known issues
The left collapser icon doesn't change in Firefox, as Firefox has no support for:has(+ sibling)
CSS selectors. This could be replaced with JavaScript code, but I like to avoid JS, and the visual impact of this issue is minor.@steven-johnson @antonysigma @maaz139 @abadams were all involved in this discussion at some point.