Wasmtime GitHub notifications bot (May 03 2022 at 06:24): Wasmtime GitHub notifications bot (May 03 2022 at 06:24):github-actions[bot] commented on issue #4093:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (May 05 2022 at 14:53):uweigand commented on issue #4093:
I was worried that this might impact the size of the generated Rust code or its runtime, but In fact, to my initial surprise (but it makes sense given the above "rarely used" factor), the generated code with this compiler fix is _exactly the same_. I rebuilt with
--features rebuild-isle,all-archbut... there were no diffs to commit! This is to me the simplest evidence that we didn't really need that complexity.I think this may have been simply because the input files didn't change and therefore the build was skipped?
In fact, if I delete the generated files and then re-generate them, they are significantly different. And at least on s390x, with the re-generated files codegen seems broken (even
cargo testin the cranelift/codegen directory already fails).
Wasmtime GitHub notifications bot (May 05 2022 at 15:55):cfallin commented on issue #4093:
Urgh, sorry about that... I should have realized the hashing mechanism would prevent the actual regen. I suspect it has to do with an implicit rule ordering (one rule isn't correct on its own) in s390x. We can revert this patch in isolation I think; nothing subsequent depends on it. Or if you're able to find the issue in s390x, that may actually be better.
I'm actually out-of-office (and with phone only, no laptop) for the rest of the week but can deal with this on Monday if someone else isn't able to first.
Wasmtime GitHub notifications bot (May 05 2022 at 16:13):uweigand commented on issue #4093:
I'll see if I can find anything. But to be clear, the generated files for all three targets changes significantly, so I don't think it's just a matter of a single s390x rule. I haven't checked if the changes on other platforms affect codegen, through.
Wasmtime GitHub notifications bot (May 05 2022 at 16:17):uweigand commented on issue #4093:
Oh, it turns out the regenerated aarch64 file doesn't even build:
error[E0317]: `if` may be missing an `else` clause --> cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.rs:3381:33 | 3381 | / ... if let Some(pattern13_0) = closure13() { 3382 | | ... // Rule at src/isa/aarch64/inst.isle line 1993. 3383 | | ... let expr0_0 = C::put_in_reg(ctx, pattern2_0); 3384 | | ... let expr1_0 = C::put_in_reg(ctx, pattern8_0); ... | 3393 | | ... return Some(expr2_0); 3394 | | ... } | |_______________________^ expected `()`, found enum `Option` | = note: expected unit type `()` found enum `Option<reg::Reg>` = note: `if` expressions without `else` evaluate to `()` = help: consider adding an `else` block that evaluates to the expected type
Wasmtime GitHub notifications bot (May 05 2022 at 16:26):cfallin commented on issue #4093:
Yep, we should revert for now; I'll reland a fixed version when I'm back. Sorry again!
As an aside this means that our "regen ISLE" CI job isn't actually doing what we think it's doing -- we should fix that too. (Probably by rm'ing the manifest files first.)
Wasmtime GitHub notifications bot (May 05 2022 at 16:30):cfallin commented on issue #4093:
@fitzgen could you put up a PR to revert? I'll r+ from here if I see it in time...
Wasmtime GitHub notifications bot (May 05 2022 at 16:42):fitzgen commented on issue #4093:
Sure thing.
Wasmtime GitHub notifications bot (May 05 2022 at 16:57):fitzgen commented on issue #4093:
Last updated: Dec 23 2024 at 12:05 UTC