cranelift-isle: Add "partial" flag for constructors #5392
Conversation
Instead of tying fallibility of constructors to whether they're either internal or pure, this commit assumes all constructors are infallible unless tagged otherwise with a "partial" flag. Internal constructors without the "partial" flag are not allowed to use constructors which have the "partial" flag on the right-hand side of any rules, because they have no way to report last-minute match failures. Multi-constructors should never be "partial"; they report match failures with an empty iterator instead. In turn this means you can't use partial constructors on the right-hand side of internal multi-constructor rules. However, you can use the same constructors on the left-hand side with `if` or `if-let` instead. In many cases, ISLE can already trivially prove that an internal constructor always returns `Some`. With this commit, those cases are laregly unchanged, except for removing all the `Option`s and `Some`s from the generated code for those terms. However, for internal non-partial constructors where ISLE could not prove that, it now emits an `unreachable!` panic as the last-resort, instead of returning `None` like it used to do. Among the existing backends, here's how many constructors have these panic cases: - x64: 14% (53/374) - aarch64: 15% (41/277) - riscv64: 23% (26/114) - s390x: 47% (268/567) It's often possible to rewrite rules so that ISLE can tell the panic can never be hit. Just ensure that there's a lowest-priority rule which has no constraints on the left-hand side. But in many of these constructors, it's difficult to statically prove the unhandled cases are unreachable because that's only down to knowledge about how they're called or other preconditions. So this commit does not try to enforce that all terms have a last-resort fallback rule.
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Instead of doing it in a separate pass afterward. This involved threading all the term flags (pure, multi, partial) through the recursive `translate_expr` calls, so I extracted the flags to a new struct so they can all be passed together.
Now that I've threaded the flags through `translate_expr`, it's easy to check this case too, so let's just do it.
There are only three legal states for the combination of `multi` and `infallible`, so replace those fields of `ExternalSig` with a three-state enum.
If we'd had any external multi-constructors this would correct their signatures as well.
I believe the only reason these weren't marked `pure` before was because that would have implied that they're also partial. Now that those two states are specified separately we apply this flag more places.
|
On a suggestion from Chris, I ran I had thought this PR might marginally improve performance in the instruction selection phase of compiling, due to fewer checks for whether constructors returned When measuring time spent in Sightglass/perf agrees with DHAT that instructions retired during compilation are improved by a statistically significant amount but with such a small effect size that it reports this PR is "1.00x faster" on the bz2, pulldown-cmark, and spidermonkey benchmarks. Sightglass' cpu-cycles measure and Hyperfine's wall-clock time both report that this PR is slightly slower when compiling Spidermonkey: the baseline version is "1.00x to 1.01x faster" according to Sightglass and "1.00 ± 0.02 times faster" according to Hyperfine. On the smaller benchmarks (bz2 and pulldown-cmark), Sightglass found no significant difference in cpu-cycles. In short, this PR has almost no effect on performance by any measure. |
Noticed this was missing, tried to add based on the comments in bytecodealliance#5392 (CC @jameysharp)
Noticed this was missing, tried to add based on the comments in #5392 (CC @jameysharp)
I speculate that this PR may have a performance impact, which I want to measure before merging, but I'd appreciate review on the implementation. Also I wanted to get the CI run going overnight.
Instead of tying fallibility of constructors to whether they're either internal or pure, this commit assumes all constructors are infallible unless tagged otherwise with a "partial" flag.
Internal constructors without the "partial" flag are not allowed to use constructors which have the "partial" flag on the right-hand side of any rules, because they have no way to report last-minute match failures.
Multi-constructors should never be "partial"; they report match failures with an empty iterator instead. In turn this means you can't use partial constructors on the right-hand side of internal multi-constructor rules. However, you can use the same constructors on the left-hand side with
iforif-letinstead.In many cases, ISLE can already trivially prove that an internal constructor always returns
Some. With this commit, those cases are laregly unchanged, except for removing all theOptions andSomes from the generated code for those terms.However, for internal non-partial constructors where ISLE could not prove that, it now emits an
unreachable!panic as the last-resort, instead of returningNonelike it used to do. Among the existing backends, here's how many constructors have these panic cases:It's often possible to rewrite rules so that ISLE can tell the panic can never be hit. Just ensure that there's a lowest-priority rule which has no constraints on the left-hand side.
But in many of these constructors, it's difficult to statically prove the unhandled cases are unreachable because that's only down to knowledge about how they're called or other preconditions.
So this commit does not try to enforce that all terms have a last-resort fallback rule.