Wasmtime GitHub notifications bot (Nov 05 2021 at 20:37): Wasmtime GitHub notifications bot (Nov 05 2021 at 20:37):alexcrichton commented on issue #3506:
For some user-feedback information, my understanding was that one of the major benefits of ISLE is to make lowerings easier and more understandable. One of the knock-on-effects of this I was curious to poke at today was to see if this applies to any of our fuzz bugs. I know very little about the existing backends, so I was curious if someone like me could jump in and "just fix the issue" by "simply translate to ISLE". To that end I chose https://github.com/bytecodealliance/wasmtime/issues/3327 since it only related to 2 instructions. I had no idea what the bug was before I started.
The tl;dr; is that translating to ISLE did indeed "simply fix the bug", but not necessarily in the most obvious way you might expect. The precise bug is that the
v128.notlowering uses anxoragainst an all-ones value, but the all-ones value is created with acmp-against-itself. This same pattern of creating an all-ones value was actually already done by fabs (somewhat ironically) which has a specific special case for floats that thebnotimplementation forgot. In that sense it was less so "moving to ISLE simply fixed things" and moreso "ISLE encouraged me to use idioms of code reuse and less open-coding" which fixed the bug.Overall I found it surprisingly easy to get up-and-running with ISLE. I made one mistake early on which resulted in a bug in the ISLE compiler but other than that I found it quite easy to envision how to translate the handwritten lowerings to ISLE. I had a lot of complexity to boot-up on related to x86_64 and how SSE works but none of that was related to ISLE, and overall the ISLE I produced in the commit felt clear, easy to read, and easy to understand.
I hope this adds at least a concrete data point of someone who knows very little about the backends was able to pretty easily fix an outstanding codegen bug simply by translating things to ISLE in the long run. I personally felt that the idioms in the
*.islefiles that @fitzgen and @cfallin have pioneered contribute a lot to this in encouraging code-reuse and type-safety, making it that much harder for a bug like this to happen again.
Wasmtime GitHub notifications bot (Nov 05 2021 at 21:01):cfallin commented on issue #3506:
@alexcrichton Thanks for giving the new framework some real-world testing with that bug! I'm very happy to hear that the secondhand effects of making small helpers and factoring easier have actually worked out here; that was indeed the goal of having a concise language, where we are not distracted by a lot of repetitive boilerplate.
I see in your commit that the fix involved expanding out the glue (support for another instruction format) as well as the actual helpers and patterns; for anyone else looking at this diff, I'll note that an eventual goal is to auto-generate more of that glue; and once most of the backend is in ISLE, we'll have a large library of helpers for all of the instructions and other common idioms, so adding specific lowering patterns should look mostly like just the ~10 lines of code added to
lower.isle.Really excited to see this coming together!
Wasmtime GitHub notifications bot (Nov 11 2021 at 17:55):fitzgen commented on issue #3506:
I did some benchmarking overnight comparing my
islebranch to the point atmainwhere it branches off, and the results are quite promising.Using
sightglass, I measured compilation, instantiation, and execution times on thebz2,pulldown-cmark,blake3-scalar, andmeshoptimizerbenchmark programs. I've removed the instantiation times from the report, because there was no difference at all there, as expected, since this doesn't touch any runtime bits.<details><summary>Benchmark Results</summary>
compilation :: cycles :: benchmarks-next/blake3-scalar/benchmark.wasm Δ = 1249090.92 ± 724010.95 (confidence = 99%) isle.so is 1.00x to 1.01x faster than main.so! main.so is 0.99x to 1.00x faster than isle.so! [246073027 252004761.30 393516494] isle.so [246875850 253253852.22 390821062] main.so compilation :: cycles :: benchmarks-next/pulldown-cmark/benchmark.wasm Δ = 2702104.27 ± 2623660.49 (confidence = 99%) isle.so is 1.00x to 1.01x faster than main.so! main.so is 0.99x to 1.00x faster than isle.so! [553373817 631555570.96 817777100] isle.so [555284228 634257675.23 814925616] main.so execution :: cycles :: benchmarks-next/pulldown-cmark/benchmark.wasm No difference in performance. [8041219 9077421.76 15404683] isle.so [8040123 9274696.62 19569691] main.so execution :: cycles :: benchmarks-next/blake3-scalar/benchmark.wasm No difference in performance. [531329 685571.44 1555546] isle.so [531327 695009.40 1930343] main.so compilation :: cycles :: benchmarks-next/bz2/benchmark.wasm No difference in performance. [353160623 373074866.11 579727603] isle.so [350066346 371869076.79 582214244] main.so compilation :: cycles :: benchmarks-next/meshoptimizer/benchmark.wasm No difference in performance. [1044734883 1084166597.80 1249172663] isle.so [1040103787 1082768758.68 1249635898] main.so execution :: cycles :: benchmarks-next/bz2/benchmark.wasm No difference in performance. [105764501 116889484.52 147028423] isle.so [106359186 117030039.58 152651625] main.so execution :: cycles :: benchmarks-next/meshoptimizer/benchmark.wasm No difference in performance. [22859199254 23059177615.72 24392758675] isle.so [22875731365 23058240476.01 23753664404] main.so</details>
TLDR:
- ISLE provides ~0.5% faster compile times for
blake3-scalarandpulldown-cmark- There are no statistically significant differences in compile times for the other benchmarks (
bz2andmeshoptimizer)- There are no statistically significant differences in execution times for any benchmark
This provides strong evidence that ISLE is a small win for compile times--certainly not a regression--and that there is also no regression in code quality and execution times.
Wasmtime GitHub notifications bot (Nov 15 2021 at 17:31):fitzgen commented on issue #3506:
@cfallin very helpfully did some git history juggling and now we have the full history for
islein this PR. This should make it easier to see who wrote which code, and easier for me to review @cfallin's work and him to review mine.On that note, over the last few weeks I've been familiarizing myself with the ISLE compiler and the code is very high quality. For the few things it wasn't handling super well (like when we keep type checking to find more errors after the first error has been found, but then run into missing state that "should" be filled in for error-free programs but wasn't because of the first error, and then we panic) I've made commits to address them and a fuzz target to exercise those code paths. So consider this my r+ stamp of code review approval for the ISLE compiler code that @cfallin wrote.
Last updated: Oct 23 2024 at 20:03 UTC