Wasmtime GitHub notifications bot (May 09 2022 at 21:32): Wasmtime GitHub notifications bot (May 09 2022 at 21:32):cfallin opened PR #4117 from isle-prio-trie-fix-round-two to main:
This PR re-applies the commit from #4093, which fixed a priority trie
build issue in the ISLE compiler. The bug affected the order in which the
rules were applied, such that specified priorities were not always
respected. (As long as rules obeyed the "any rule in isolation is correct"
principle, no correctness issues would result, but it is a real bug that
makes developing new rules sometimes counterintuitive.)Unfortunately, in #4102 we had to revert that commit because it
caused the ISLE compiler to produce incorrect code. Furthermore,
I didn't catch this in the original PR (during development or in CI)
because both our CI, and my mental model of "rebuilding ISLE source",
did not account for therebuild-islestill skipping the rebuild if
the manifest was up-to-date.(This is a point in favor of doing something about confusion in #4066,
but that's a separate discussion!)It turns out that the bug in #4093 was quite simple in hindsight: we
only need to recurse down the trie and sort edges in subnodes too.
I should have caught this but didn't because my test case (for the
original bug) involved mis-sorted edges only in a one-level-above-leaf
node.This PR re-applies the original priority trie fix, with a fix to the fix
(the recursive application of sorting). It also factors out the "force
rebuild" step into a shell script for easier use by ISLE-compiler
developers.The generated code changes in exactly one spot in the x64 backend,
with alet-binding moving downward by 18 lines. That's it!
aarch64ands390xdo not change at all. So, no performance
or other behavior changes are expected as a result of this.
Wasmtime GitHub notifications bot (May 09 2022 at 21:32):cfallin requested fitzgen for a review on PR #4117.
Wasmtime GitHub notifications bot (May 09 2022 at 22:18):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 22:18):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 22:18):fitzgen created PR review comment:
We can future proof this a little for when we have more ISLE than just lowering.
rm -f cranelift/codegen/**/generated_code.manifest
Wasmtime GitHub notifications bot (May 09 2022 at 22:26):cfallin created PR review comment:
Hmm, the double-asterisk wildcard seems to work in bash and zsh, but not dash (Ubuntu's default /bin/sh [1]) or 2007-era bash on my macOS machine. So there is no portable shebang line that specifically gets bash on Ubuntu and zsh on macOS.
[1] https://wiki.ubuntu.com/DashAsBinSh
I'll add a comment to the build script instead to note that this should be updated. Thanks for raising the issue in any case!
Wasmtime GitHub notifications bot (May 09 2022 at 22:26):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 22:29):cfallin updated PR #4117 from isle-prio-trie-fix-round-two to main.
Wasmtime GitHub notifications bot (May 09 2022 at 22:34):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 22:34):fitzgen created PR review comment:
Can we use a
#!/usr/bin/env bashshebang to reliably get bash across platforms?
Wasmtime GitHub notifications bot (May 09 2022 at 23:01):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 23:01):cfallin created PR review comment:
The issue is that bash on macOS is from 2007 (because GPL 3 I guess) and doesn't have the feature, so we need a different shell on different platforms (
zshon modern macOS,bashon Linux).Other environments may not have a new enough shell either (older Linuxes, etc) and I don't really want to introduce a special dependency. Or at least, this isn't really a good enough reason to do so, IMHO.
Wasmtime GitHub notifications bot (May 09 2022 at 23:36):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 09 2022 at 23:36):cfallin created PR review comment:
(merging now with your r+ but happy to refine this later if you feel strongly about it; probably would need a Rust script in place of the nonportable wildcard)
Wasmtime GitHub notifications bot (May 09 2022 at 23:36):cfallin merged PR #4117.
Last updated: Dec 23 2024 at 12:05 UTC