Stream: git-wasmtime

Topic: wasmtime / PR #6412 x64: Update all feature tests to pure...

Wasmtime GitHub notifications bot (May 18 2023 at 18:49):

alexcrichton opened PR #6412 from alexcrichton:x64-feature-names to bytecodealliance:main:

Many of the tests for lowering features use extractors I think since when they were added that was the only option. Nowadays a pure constructor with if-let feels more natural for this, so this commit updates all existing extractors used to test features to use a pure constructor instead. This additionally updates the names to match the cranelift codegen setting name to ensure that a consistent name is used.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 18 2023 at 18:49):

alexcrichton has marked PR #6412 as ready for review.

Wasmtime GitHub notifications bot (May 18 2023 at 18:49):

alexcrichton requested jameysharp for a review on PR #6412.

Wasmtime GitHub notifications bot (May 18 2023 at 18:49):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6412.

Wasmtime GitHub notifications bot (May 18 2023 at 18:59):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 18 2023 at 19:26):

fitzgen submitted PR review:

Nice, yes this stuff did indeed come before pure constructors and if-let.

Wasmtime GitHub notifications bot (May 18 2023 at 19:27):

jameysharp submitted PR review:

Excellent idea. Among other things, this gives ISLE a lot more freedom to schedule these feature tests with respect to other pattern matches, since it no longer believes there's a data dependency on the type. I like to diff the ISLE-generated source before and after changes like this just because I think it's interesting to see how the schedule changes.

I'd love to see some Sightglass figures for the compile phase. It should be no worse, and possibly better, but ISLE uses a pretty simple greedy heuristic for match ordering so anything is possible.

Wasmtime GitHub notifications bot (May 18 2023 at 19:27):

jameysharp submitted PR review:

Excellent idea. Among other things, this gives ISLE a lot more freedom to schedule these feature tests with respect to other pattern matches, since it no longer believes there's a data dependency on the type. I like to diff the ISLE-generated source before and after changes like this just because I think it's interesting to see how the schedule changes.

I'd love to see some Sightglass figures for the compile phase. It should be no worse, and possibly better, but ISLE uses a pretty simple greedy heuristic for match ordering so anything is possible.

Wasmtime GitHub notifications bot (May 18 2023 at 19:27):

jameysharp created PR review comment:

I have a preference for using pattern-matches rather than priorities whenever possible.

In this case I could be convinced the other way: do_ctz is more general and still works even if use_bmi1 is enabled, so semantically the priorities make sense; and it probably doesn't make much difference in terms of LLVM's output.

But in general, priorities are kinda weird action-at-a-distance, and they can impede ISLE optimizations.

Wasmtime GitHub notifications bot (May 18 2023 at 20:08):

fitzgen merged PR #6412.

Wasmtime GitHub notifications bot (May 18 2023 at 21:30):

alexcrichton created PR review comment:

Ah true! I did this as a drive-by fix since this was the only location (plus one other here in this PR) that explicitly matched $false where all other locations which have feature-specific lowerings only match on $true. Do you know how hard it would be to remove the need for priorities here? For example something like auto-prioritizing "some if-let" over "no if-let" and then requiring an explicit priority if two rules have if-let? Basically some way to automatically handle this case with good codegen and no priorities.

Wasmtime GitHub notifications bot (May 18 2023 at 22:59):

jameysharp created PR review comment:

ISLE used to treat more "specific" rules as implicitly higher priority, but we've been moving away from this kind of implicit priority scheme because it was hard for rule authors to reason about. We have all the code needed to implement it in ISLE if we wanted to, though; it's just an application of the subset-overlap check.

If you use if-let in both rules, matching against $true and $false explicitly, you'll get good codegen without priorities, and the overlap check will understand that the two rules are mutually exclusive. But as I said that's a little unfortunate here where the $false case would still be correct if the $true case is removed. So maybe this is just fine as-is.

Wasmtime GitHub notifications bot (May 19 2023 at 14:12):

alexcrichton created PR review comment:

Yeah I realize the implicit priority scheme had its own issues but my hope was that "zero if-let" vs "one or more if-let" is pretty clear in terms of priority to not need an explicit disambiguation. So not the full "which if-let is more specific", just a zero-vs-nonzero check. My hope is that the idiom of "write a rule with if-let" ideally wouldn't require explicit priorities and could be handled in the common case because it's nice to not have to update all other lowerings when a more explicit one is added.

Last updated: Dec 23 2024 at 12:05 UTC