wasmtime / PR #3506 Initial ISLE integration for x64 · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #3506 Initial ISLE integration for x64

Wasmtime GitHub notifications bot (Nov 03 2021 at 23:27):

fitzgen opened PR #3506 from isle to main:

This pull request introduces initial ISLE integration for the x64 backend. It is blocked on https://github.com/bytecodealliance/rfcs/pull/15 finishing its final comment period and being accepted.

I'm opening this up early since (a) I have a bunch of working code here that already ports a significant amount of the hand-written clif-to-MachInst lowering for x64, and (b) so that folks who are interested in how ISLE will work can get a chance to dig into more details and actual code rather than just slides I've presented at Cranelift meetings.

I'll keep pushing to this branch/PR as I continue porting more hand-written lowering code to ISLE.

Feel free to comment and give feedback or ask questions!

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

Similarly, I'd find it clearer to have the SSE version after all the GPR versions.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

Some error implementations have more helpful messages in their debug output than in their Display output, so I suggest using {:?} instead of {} here.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

Should this be imul instead of iadd? And similar below.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

As a minor style suggestion, I'd find it easier to read if the i64 and smaller code was before the i128 code. That way, reading this top-to-bottom, I can start with the simple form, figure out what that's doing, and then look at the i128 version and see how it differs in the obvious way.

And similar for the patterns below.

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

Is the with_type a predicate that determines whether the pattern matches? Could I suggest the name has_type for this?

Wasmtime GitHub notifications bot (Nov 04 2021 at 00:12):

sunfishcode created PR review comment:

It's not obvious to me what this <ty syntax means.

Wasmtime GitHub notifications bot (Nov 04 2021 at 01:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 01:32):

cfallin created PR review comment:

This is an "in-arg" to the left-hand-side extractor; the basic idea is that while the arg-positions of left-hand-side patterns are mostly "outputs" of whatever the matcher is doing (the inverse of a function application on the RHS), some matcher implementations will want an input (in addition to the thing they're matching on) to configure how they match. So here I think simm32_from_value is a pattern-matcher that is parameterized on a type.

(If one were doing this in e.g. Prolog it'd be just another argument, and dataflow could flow either way during unification. Here we have "argument polarity" instead, which one can think of as an approximation of this...)

I did mostly intend this to be a low-level feature that we could use to define other sorts of extractors/matchers...

@fitzgen: I wonder if there's a way to better write this? It looks like the ty in-arg eventually gets down to to_simm32 here which just checks if ty is less than 64 bits, and skips an "is sign extendable" check if so; but it looks like this may actually be unnecessary (I don't fully remember the semantics of simm32 values on 32 vs 64 bit instructions though)? Or could we just split out the 8/16/32 and 64 bit cases (one of the advantages of just having a list of patterns even!)?

Wasmtime GitHub notifications bot (Nov 04 2021 at 01:34):

cfallin created PR review comment:

Alternately, we propagate arg polarity to the parser and parse in-args as expressions without the <, but then that makes this a type-dependent parse (a la C++) which is kind of not great. We could do it though if we require decls to come before uses of terms...

Wasmtime GitHub notifications bot (Nov 04 2021 at 01:34):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:05):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:05):

bjorn3 created PR review comment:

This unconditionally writes to the source dir. Crates.io dependencies aren't allowed to write to their source dir. cargo publish checks that the source dir stays identical. If the isle file is up to date, this check will currently pass, but if in the future the source dirs are made readonly, this will break.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:08):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:08):

bjorn3 created PR review comment:

Maybe write it to OUT_DIR too and let ISLE read it from there?

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:13):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:13):

bjorn3 created PR review comment:

The pre-regalloc printing now doesn't make sense anymore. After regalloc it will hide invariant violations.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:15):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 13:15):

bjorn3 created PR review comment:

This is unconditionally using isle, right? This shouldn't be merged without benchmarking first.

Wasmtime GitHub notifications bot (Nov 04 2021 at 17:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 17:11):

cfallin created PR review comment:

I wonder if the checked-in-generated-code approach could be done as follows: (i) provide a shell-script that regenerates all relevant source; (ii) in CI, fail if generated source is not up-to-date; (iii) do nothing in build.rs, to avoid surprising side-effects in the source tree.

This seems to be the simplest and fastest option, and keeps to the general principle that the source tree is human-managed and not edited by tools except by explicit invocation of a command. It does however require a manual ./update-isle.sh && cargo build when iterating on the rules. Maybe a way to avoid a footgun here is to error in build.rs if the generated source is out of data. We could do that without putting islec in the build deps by recording a hash of the ISLE source alongside generated code, and checking against that hash in build.rs. Thoughts?

Wasmtime GitHub notifications bot (Nov 04 2021 at 17:11):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:10):

fitzgen created PR review comment:

Is the with_type a predicate that determines whether the pattern matches?

Yep

Could I suggest the name has_type for this?

Nice, its even a character shorter

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:22):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:22):

fitzgen created PR review comment:

Or could we just split out the 8/16/32 and 64 bit cases (one of the advantages of just having a list of patterns even!)?

What's nice about having polymorphic rules instead of a separate rule for each of i{8,16,32,64} is that they actually end up as a single branch in the trie/generated code, instead of four different branches.

I wonder if there's a way to better write this? It looks like the ty in-arg eventually gets down to to_simm32 here which just checks if ty is less than 64 bits, and skips an "is sign extendable" check if so; but it looks like this may actually be unnecessary

Yeah I think we could just always do the "is sign extendable" check and it would be equivalent.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:22):

sunfishcode created PR review comment:

Ah, I see. Fwiw, I'd find =ty more intuitive syntax for this than <ty.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:28):

fitzgen created PR review comment:

Whoops, good catch!

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:28):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:34):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:34):

fitzgen created PR review comment:

I plan to add a CI check for whether the ISLE code is up-to-date, will implement that soon.

I think it would be easier to add a rebuild-isle feature to cranelift-codegen-meta than to do all of that with the hashing and everything.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:35):

fitzgen created PR review comment:

Yes, expanding formatting is something to do eventually, but I don't think it needs to block this PR.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:35):

fitzgen created PR review comment:

Yes, that's the plan.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:44):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:44):

cfallin created PR review comment:

I think, at least the way I'm imagining it, the rebuild (rebuild-isle feature sounds good, I guess that's an implementation strategy for an update-isle.sh or moral equivalent?) and the local-development-loop check (using hashing or something else) are orthogonal.

Basically, (i) the rebuild is an explicit action that the user invokes in order to update the in-tree generated source, and (ii) hashing and checking in build.rs is one strategy of ensuring that a local build doesn't succeed unless the user does this explicit update.

My thinking comes from two principles -- first, that we shouldn't have a setup where an edit to a .isle file is ignored during a rebuild if one forgets to do a manual step (otherwise, we're bound to waste time debugging why a change didn't make a difference...) and second, that it's kind of an anti-pattern to have a build step that automatically updates a file in src/, without the user asking for it. So from those two principles, we have that (i) there should be a separate update step, and (ii) we should check that it was done.

Possibly those aren't principles that we want, but they seem to me to be a reasonable starting place; thoughts?

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:48):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:52):

bjorn3 created PR review comment:

Can these be inlined into the generated code? That should reduce compile times and improve debug mode performance. (The latter is important for cg_clif as CI compiles rustc using cg_clif and then builds a sysroot using it as test.)

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:52):

bjorn3 created PR review comment:

This is a codegen regression, right?

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:52):

bjorn3 created PR review comment:

There is a lot of inefficiencies in the generated code. While an optimizer can fix it, this only happens in release mode and increases build time.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 18:57):

cfallin created PR review comment:

That would be a major rearchitecting of the ISLE DSL compiler -- it would require us to carry around Rust code as strings and inject it, and have a general language feature for this, which IMHO at least is a thing we want to avoid.

The basic idea of the integration between DSL code and Rust code is that we're able to define helpers like these, and then call them; blending the boundary further is I think a recipe (as it were) for complexity and pain...

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:00):

cfallin created PR review comment:

It's true that the DSL compiler has a simple emission strategy, but I don't think this sort of thing specifically actually results in worse code. Basically we are taking a compound expression and pulling it apart into separate let-bindings; afaik the MIR from this should be identical.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:01):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:01):

fitzgen created PR review comment:

=variable is already used for unifying two variables, eg
(rule (has_type ty (ixor x =x))
    (imm ty 0))

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:06):

cfallin created PR review comment:

As a higher-level point, while I definitely understand the request, I don't think that "optimize for performance when the inliner doesn't inline anything" is a reasonable principle from a broader point of view: it is basically saying that we cannot use abstraction or helper functions, because the toolchain might have a bad worst-case behavior. IMHO it's reasonable to assume we can write single-line helpers, and sacrificing expressivity (even basic principles like extracting helper functions) in order to speed up the software in a debug build seems like a poor tradeoff, and one that is better fixed by finding ways to build software in fast-but-lightly-optimized ways (which is, incidentally, what cg_clif is meant to solve if I'm not mistaken!).

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:07):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:07):

fitzgen created PR review comment:

I don't think so: these used to be the mov $$0, %regs in the branches and correctly get turned into xor %reg, %reg now.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:07):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:08):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:09):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:09):

fitzgen created PR review comment:

I don't think that "optimize for performance when the inliner doesn't inline anything" is a reasonable principle

Strong agree

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:09):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:09):

cfallin created PR review comment:

@fitzgen since we're hoping it will mostly be a lower-level thing that we build on top of, we could make the syntax more explicit, maybe: what do you think about something like (eval ...) or (matcher-input ...)?

(Basically, I too would be happy to see <ty die; it was literally just the first punctuation character I thought of and I wish I had chosen something better initially!)

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:10):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:12):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:12):

bjorn3 created PR review comment:

MIR doesn't do copy propagation. Every assignment results in a copy at MIR level.

Current code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8cf97ba023221ffafccb62ab89231fab

Excerpt:

    bb0: {
        _5 = move _2;                    // scope 0 at src/lib.rs:21:22: 21:26
        _6 = move _3;                    // scope 1 at src/lib.rs:22:22: 22:26
        _7 = _4;                         // scope 2 at src/lib.rs:23:22: 23:26
        discriminant(_8) = 0;            // scope 3 at src/lib.rs:25:19: 25:50
        _12 = &mut (*_1);                // scope 4 at src/lib.rs:26:39: 26:42
        _13 = move _5;                   // scope 4 at src/lib.rs:26:44: 26:54
        _15 = &_8;                       // scope 4 at src/lib.rs:26:56: 26:64
        _14 = _15;                       // scope 4 at src/lib.rs:26:56: 26:64
        _16 = move _6;                   // scope 4 at src/lib.rs:26:66: 26:76
        _17 = _7;                        // scope 4 at src/lib.rs:26:78: 26:88
        _11 = constructor_shift_r::<C>(move _12, move _13, move _14, move _16, move _17) -> bb1; // scope 4 at src/lib.rs:26:19: 26:89
                                         // mir::Constant
                                         // + span: src/lib.rs:26:19: 26:38
                                         // + literal: Const { ty: for<'r, 's, 't0> fn(&'r mut C, Type, &'s ShiftKind, Reg, &'t0 Imm8Reg) -> std::option::Option<Reg> {constructor_shift_r::<C>}, val: Value(Scalar(<ZST>)) }
    }

With copies eliminated: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=61312e466966b68bd721e61f98cc62e3

    bb0: {
        _8 = &mut (*_1);                 // scope 0 at src/lib.rs:21:39: 21:42
        _9 = move _2;                    // scope 0 at src/lib.rs:21:44: 21:48
        _19 = const constructor_sar::<C>::promoted[0]; // scope 0 at src/lib.rs:21:50: 21:82
        _11 = _19;                       // scope 0 at src/lib.rs:21:50: 21:82
        _10 = _11;                       // scope 0 at src/lib.rs:21:50: 21:82
        _12 = move _3;                   // scope 0 at src/lib.rs:21:84: 21:88
        _13 = _4;                        // scope 0 at src/lib.rs:21:90: 21:94
        _7 = constructor_shift_r::<C>(move _8, move _9, move _10, move _12, move _13) -> bb1; // scope 0 at src/lib.rs:21:19: 21:95
                                         // mir::Constant
                                         // + span: src/lib.rs:21:19: 21:38
                                         // + literal: Const { ty: for<'r, 's, 't0> fn(&'r mut C, Type, &'s ShiftKind, Reg, &'t0 Imm8Reg) -> std::option::Option<Reg> {constructor_shift_r::<C>}, val: Value(Scalar(<ZST>)) }
    }

Something that would probably help a lot more though is avoiding ? and instead using match. ? desugars into several function calls:

    bb1: {
        _7 = <Option<Reg> as Try>::branch(move _8) -> bb2; // scope 1 at src/lib.rs:22:19: 22:68
                                         // mir::Constant
                                         // + span: src/lib.rs:22:19: 22:68
                                         // + literal: Const { ty: fn(std::option::Option<Reg>) -> std::ops::ControlFlow<<std::option::Option<Reg> as std::ops::Try>::Residual, <std::option::Option<Reg> as std::ops::Try>::Output> {<std::option::Option<Reg> as std::ops::Try>::branch}, val: Value(Scalar(<ZST>)) }
    }

    bb2: {
        _15 = discriminant(_7);          // scope 1 at src/lib.rs:22:19: 22:68
        switchInt(move _15) -> [0_isize: bb3, 1_isize: bb5, otherwise: bb4]; // scope 1 at src/lib.rs:22:19: 22:68
    }

    bb3: {
        _18 = move ((_7 as Continue).0: Reg); // scope 1 at src/lib.rs:22:19: 22:68
        _6 = move _18;                   // scope 6 at src/lib.rs:22:19: 22:68
        _19 = move _6;                   // scope 2 at src/lib.rs:23:17: 23:24
        ((_0 as Some).0: Reg) = move _19; // scope 2 at src/lib.rs:23:12: 23:25
        discriminant(_0) = 1;            // scope 2 at src/lib.rs:23:12: 23:25
        goto -> bb6;                     // scope 2 at src/lib.rs:23:5: 23:25
    }

    bb4: {
        unreachable;                     // scope 1 at src/lib.rs:22:19: 22:68
    }

    bb5: {
        _0 = <Option<Reg> as FromResidual<Option<Infallible>>>::from_residual(move _17) -> bb6; // scope 4 at src/lib.rs:22:19: 22:68
                                         // mir::Constant
                                         // + span: src/lib.rs:22:67: 22:68
                                         // + literal: Const { ty: fn(std::option::Option<std::convert::Infallible>) -> std::option::Option<Reg> {<std::option::Option<Reg> as std::ops::FromResidual<std::option::Option<std::convert::Infallible>>>::from_residual}, val: Value(Scalar(<ZST>)) }
    }

    bb6: {
        return;                          // scope 0 at src/lib.rs:24:2: 24:2
    }

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:25):

cfallin created PR review comment:

OK, interesting. Echoing the point I made above in another thread, though, this seems to me to be more a problem that debug builds of Rust do no optimization whatsoever (and this is a problem for any Rust code that is written with abstractions or redundancies that optimize away), and less that our particular DSL codegen produces code that has optimizable parts.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:30):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:30):

bjorn3 created PR review comment:

Replacing expr? with if let Some(x) = expr { x } else { return None; } should be an easy change in the codegen. The other suggested changes are not worth it I guess, though they will help with improving build time.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:40):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 19:40):

sunfishcode created PR review comment:

I agree that this kind of thing could end up being significant, especially as we add more patterns to isle and the generated files grow. However, optimizations like these complicate the generator, and right now, keeping that code simple is particularly valuable, as the code is new and still rapidly changing.

As ISLE matures, we add more patterns, and the generated files become large enough that effects like this are measurable, I expect it'll make sense to revisit this.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:07):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:07):

fitzgen created PR review comment:

simm32 immediates no longer need input parameters, fwiw. They were the only use of input args to extractors in all of the lowering code.

One thing I was thinking, and maybe this is wild, was to always have extractors use [...] and always have expressions use (...) but this doesn't really solve the case where a simple, non-operation expression is an input.

Or maybe, since we aren't using input polarity arguments for extractors anymore, we simply don't need them and can remove them from the language?

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:10):

fitzgen created PR review comment:

So from those two principles, we have that (i) there should be a separate update step, and (ii) we should check that it was done.

Agreed with these principals.

I'm not convinced we have to do more than what this PR currently does in this PR though.

Like I think there is a separate chunk of work that removes all of cranelift-codegen-meta from cranelift-codegen's dependency tree and does the all things you're talking about. But for now, the ISLE integration basically does what we already do for the rest of cranelift-codegen-meta, and I think that is fine for landing the initial ISLE integration.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:27):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:31):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:31):

cfallin created PR review comment:

Ah, OK, possibly I'm not understanding then -- from what I can tell in this PR, the rebuild-isle feature is non-default, and if I (i) do a clean git-clone, (ii) modify some ISLE source, and (iii) cargo build, without specifying the feature or doing anything else, my local build doesn't pick up the new ISLE changes -- is that right? Or is there some plumbing somewhere that takes care of that?

This is basically the case that motivates what I had suggested above -- I think that we need a check not just in CI, but locally, that the checked-in code matches what would have been generated (but only actually overwrite if the user opts-in, with a feature or script or whatnot). To do otherwise seems to me to invite lots of confusion the first time a new contributor tries to add a lowering and wonders why it doesn't work :-)

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:34):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:34):

sunfishcode created PR review comment:

It may be different in terms of what's actually going on in the DSL, however as someone just reading the code, this =x and the <ty above kinda feel like they're doing the same conceptual thing. Whether it's a type or a variable, the prefix means "don't bind a new thing here, reuse an existing binding". So I wonder if it would work to use prefix-= syntax for both.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 20:56):

fitzgen created PR review comment:

Yeah if you don't build with --features rebuild-isle then it won't rebuild ISLE or tell you to use that feature right now.

I think this is fine for this PR (since we do check for and catch this in CI in this PR) and then in a future PR we can redo the whole paradigm for how we do meta, compiler compile time.

Wasmtime GitHub notifications bot (Nov 04 2021 at 21:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 21:04):

cfallin created PR review comment:

OK; I agree it's fine for now (first PR) but it seems like something that's likely to bite us pretty soon after, just as soon as others start to use it. I suspect it could be addressed in ~50 lines or so of code in build.rs, without requiring us to build the ISLE compiler in the default build, just hashing files and either reading/comparing or writing to a checked-in "manifest". I'm happy to do so once the initial thing merges. In the meantime could you add a TODO somewhere with a tracking issue?

Thanks for the patience in working this one out!

Wasmtime GitHub notifications bot (Nov 04 2021 at 21:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2021 at 21:06):

fitzgen created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/3508

Wasmtime GitHub notifications bot (Nov 04 2021 at 22:00):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 22:43):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 23:02):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 23:05):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 23:42):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 04 2021 at 23:57):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 05 2021 at 10:01):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2021 at 10:01):

bnjbvr created PR review comment:

Wouldn't there be a way to have very explicit syntax with another sub-expression or an annotation, instead of using an operator which significance has to be looked up in documentation and learned about? Ad-hoc syntax isn't self-explanatory, and as a matter of fact it is spurious extra complexity for newcomers, especially if it _can_ be avoided by using another more explicit syntax.

Wasmtime GitHub notifications bot (Nov 05 2021 at 15:54):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2021 at 15:54):

cfallin created PR review comment:

Agreed with @bnjbvr; I'd like to re-boost my suggestion above for (matcher-input ...) if we end up needing this feature still. I'm also hoping that, if it remains, we can keep its use confined to just whatever "standard library helpers" need it, if that, and keep it out of ordinary patterns entirely.

Also agreed with @fitzgen that given that we found a way to avoid it here, maybe we can remove it; it nudges the language more toward "general-purpose constraint solver" (ie the Prolog direction) and less toward "patterns of instructions", when the latter is really the simplicity and approachability we're going for, so with whatever influence I still have on the language design, I'd like to push to undo my mistake :-)

Wasmtime GitHub notifications bot (Nov 05 2021 at 17:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2021 at 17:29):

fitzgen created PR review comment:

For sure. I'm not going to deal with it right now, in this PR, but if we don't end up removing this feature from the DSL completely, we can use (eval ...) or (matcher-input ...) or something.

Wasmtime GitHub notifications bot (Nov 05 2021 at 17:32):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 05 2021 at 17:47):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 05 2021 at 17:47):

bjorn3 created PR review comment:

Should it be versioned together with cranelift?

Wasmtime GitHub notifications bot (Nov 05 2021 at 20:46):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 05 2021 at 21:07):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 05 2021 at 23:05):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 08 2021 at 21:21):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 11 2021 at 17:40):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 11 2021 at 23:19):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:25):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:25):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:32):

fitzgen has marked PR #3506 as ready for review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton created PR review comment:

Is this step still necesary now that rustfmt is baked in?

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton created PR review comment:

I was poking around with a lowering that used this locally, and I think that this may not have been listed in the right order? I think that Idx should come after the value_array_2, right?

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton created PR review comment:

FWIW I don't think this will run on oss-fuzz unless it's in the top-level directory by default (but we could modify oss-fuzz to build here too if desired)

Wasmtime GitHub notifications bot (Nov 15 2021 at 17:53):

alexcrichton created PR review comment:

I ended up having this -- https://github.com/alexcrichton/wasmtime/commit/d0f2bf4c140730566d00319def57fa1bdbc27269 -- to get matchers on insertlane working (although I'm not 100% sure it was correct)

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin created PR review comment:

Just a small thing, but unconditionally emitting terminal control codes without tty detection is kind of unfortunate; e.g. sometimes I pipe compiler output to a file or page through it with less. Also someone who is actually using a Braille screenreader would be broken by this option. Can we make this opt-in via an environment variable or Cargo feature?

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin created PR review comment:

Idle thought: it might make sense to namespace the x86 MachInst constructors all to (x86_rotl ...), (x86_add ...), etc. -- then it's clear that we're lowering into a MachInst and not into some other intermediate form. If we did this it wouldn't have to be with this PR but we'd want to do it soon to avoid more find/replace pain later. Thoughts?

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin created PR review comment:

This can change to the Wasmtime repository now I think?

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin created PR review comment:

That's not a bad idea; @fitzgen can we call this "isle 0.78.0" along with Cranelift? We should make sure the version-bumping logic in publish.rs updates the isle/islec crates as well.

Wasmtime GitHub notifications bot (Nov 15 2021 at 18:11):

cfallin created PR review comment:

Is this file leftover from an earlier generated-code setup? I don't see where it's generated or used (but possibly I'm missing something)... and src/clif.isle is also used to generate the x64-specific lowering code so I don't think we need this separate machine-independent one?

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:10):

fitzgen created PR review comment:

This isn't emitting the terminal control codes, there is a separate config for colors, it is just whether we get pure-text error messages or whether it includes the source context snippets and unicode box characters. It works just fine piped to less or into a file.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:11):

fitzgen created PR review comment:

I had similar thoughts of using namespace:instruction like (clif:iadd ...) and (x86:add ...). Happy to bike shed in a follow up PR.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:15):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:15):

fitzgen created PR review comment:

Whoops yeah this is unused.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:15):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:15):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:15):

cfallin created PR review comment:

Ah, OK, that's much better, sorry for misunderstanding! Maybe a comment here to the effect of "Ensure miette emits source snippets, even when the output is not a tty (note there are no terminal control codes in 'graphical' output)" or somesuch would help?

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:18):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:22):

fitzgen created PR review comment:

I actually don't think we should run this fuzz target in oss-fuzz. It is useful to run locally a bit for folks poking on the ISLE compiler, but the ISLE compiler doesn't need to be hardened in the same way that Wasmtime does.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:22):

fitzgen created PR review comment:

Yes, since the rustfmting is happening on a best-effort basis, and if rustfmt isn't on $PATH it doesn't break the build.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:23):

fitzgen created PR review comment:

That looks correct to me. Do you want to make a PR after this one merges?

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:27):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:30):

cfallin created PR review comment:

Ah, actually, reading this again, I note 'unicode box characters'. I do think that making this opt-in is actually an important accessibility thing; if miette went to the trouble of supporting screenreaders, we should not undo that work.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:33):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:33):

fitzgen created PR review comment:

The problem is that their detection algorithm is broken and the errors are basically unusable without force_graphical(true) for me, the main person reading these errors.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:38):

cfallin created PR review comment:

I don't think it's either-or, though; could we just add a Cargo feature? What I'm envisioning is if cfg!(feature = "miette-errors-graphical") { ... } around this. Or an environment-variable if that's easier...

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:46):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 19:46):

fitzgen created PR review comment:

I'm not keen to add another speed bump to my own development process by requiring that I enable another env var or cargo feature that I'll forget most of the time.

How about we leave it as is for now, I file a bug upstream in miette, and then we remove this hack once miette fixes the root cause?

Wasmtime GitHub notifications bot (Nov 15 2021 at 20:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 20:46):

cfallin created PR review comment:

Yep, that sounds like a reasonable compromise! Just a FIXME comment here noting said issue would be good, then I'm happy :+1:

Wasmtime GitHub notifications bot (Nov 15 2021 at 22:54):

fitzgen updated PR #3506 from isle to main.

Wasmtime GitHub notifications bot (Nov 15 2021 at 23:38):

fitzgen merged PR #3506.

Wasmtime GitHub notifications bot (Nov 15 2021 at 23:49):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 15 2021 at 23:49):

fitzgen created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/3529

Last updated: Oct 23 2024 at 20:03 UTC