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

print: align adjacent multi-version columns' lines, to match up their anchors. #18

Merged
merged 11 commits into from
Mar 31, 2023

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Feb 14, 2023

This extra step, just before turning multi-version pretty-printed fragments into a HTML table row, allows for a clearer visual match between the columns, turning the multi-version mode into something closer to a pass "differ", but the use of anchors (instead of line contents) makes it more conservative, reliable and somewhat simpler.

In short, if two adjacent versions contain a definition like v123 = ..., it should now end up on the same horizontal line in both versions (assuming it's not e.g. out of order wrt most of the other definitions).

Before, adding or removing instructions (or even their pretty-printing needing different line amounts) would result in one side being behind (and the other, ahead), which would accumulate (even if most of their contents might be similar or even identical). With this PR, a lot of that should be alleviated, as padding (of empty lines) is added to keep both versions aligned as much as possible.


However, while it works great for correcting small misalignments (the original usecase), it's quite zealous (and lacks any heuristics or further constraint-solving etc.), leading to results like this:

Before
(click for complete
pretty HTML example)
image
After
(click for complete
pretty HTML example)
image

This is for the same SPIR-T as the last screenshot from the Rust-GPU reduce+fuse_selects PR.

The OpNops don't help (and those should be gone once SPIR-T APIs are adjusted), but it's generally much worse looking than I was hoping. Maybe it's worth it for the ability, but some things are just annoying.

Based on this screenshot alone, several possible further improvements come to mind:
(EDIT: several of these have been implemented in this PR since it was opened)

  • hidden anchors could be added for e.g. ControlNodes and ControlRegions, even if there's not necessarily any syntax to attach them to (other than the keyword for non-Block ControlNodes and the {...} brackets for ControlRegion?)
  • the contents of the aligned lines could be compared and some kind of graying/translucency used to de-emphasize the lines that haven't changed
    • there's a risk here of making them confused with removed lines
      • this is where merely partial grayscaling, or some kind of sepia effect, might come in handy
    • such diff-like mode could supplant the use of colspan entirely, and allow horizontal scrolling of the whole table, with the max line length being used to size the cells (e.g. td { max-width: 100ch; }), instead of divvying up the whole viewport width
    • may need some level of interactivity (based on which column is being hovered on?)
  • Rust-GPU's --spirt-dump-passes could skip the unstructured control-flow column, which adds a bunch of gaps for no good reason (or it could be kept but with the alignment turned off?)
  • euristics (or just more user control) to avoid large gaps
  • some additional "layout" pass could rearrange all the lines that are "floating" in between anchors
    • crucially, the current state is that the new vertical gaps are always inserted just before the anchor, and never elsewhere, but the lines before the anchor might be more "attached" to it than the ones after
      (sadly, we can't just switch where the vertical gap goes unconditionally: anchors can easily precede multiple-line contents that should not be broken up)
    • e.g. ideally the vertical gap would be at the outermost indentation level, which would allow e.g. structured control-flow to "coalesce together" (may require either using LineOp more directly, or just making indentation special in TextOp)
    • it almost feels like this requires a redesign of a bunch of the print::pretty layout, to allow "flexible vertical spacing", which frankly seems like overkill
  • dynamic control over the HTML table via JS, to allow user adjustments on the fly
    • some part of the anchor alignment algorithm would likely need to be implemented in JS, to avoid the combinatorial explosion in the data that would need to be statically generated otherwise

@eddyb

This comment was marked as resolved.

@eddyb eddyb force-pushed the multiversion-alignment branch from 9e89e3f to 912eb3c Compare February 16, 2023 01:17
@eddyb eddyb force-pushed the multiversion-alignment branch 2 times, most recently from 086105d to 29dced6 Compare March 12, 2023 16:03
@eddyb eddyb force-pushed the multiversion-alignment branch from 29dced6 to 8af09b9 Compare March 31, 2023 12:35
@eddyb eddyb marked this pull request as ready for review March 31, 2023 13:21
@eddyb eddyb added this pull request to the merge queue Mar 31, 2023
Merged via the queue into main with commit e47d428 Mar 31, 2023
@eddyb eddyb deleted the multiversion-alignment branch March 31, 2023 13:49
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.

2 participants