Stream: git-wasmtime

Topic: wasmtime / PR #4143 Cranelift: do not check in generated ...


view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:02):

cfallin requested abrown for a review on PR #4143.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:02):

cfallin opened PR #4143 from isle-no-checked-in-code to main:

This PR fixes #4066: it modifies the Cranelift build.rs workflow to
invoke the ISLE DSL compiler on every compilation, rather than only
when the user specifies a special "rebuild ISLE" feature.

The main benefit of this change is that it vastly simplifies the mental
model required of developers, and removes a bunch of failure modes
we have tried to work around in other ways. There is now just one
"source of truth", the ISLE source itself, in the repository, and so there
is no need to understand a special "rebuild" step and how to handle
merge errors. There is no special process needed to develop the compiler
when modifying the DSL. And there is no "noise" in the git history produced
by constantly-regenerated files.

The two main downsides we discussed in #4066 are:

This PR addresses each of these concerns:

  1. To maintain reasonable compile time, it includes work to cut down the
    dependencies of the cranelift-isle crate to nothing (only the Rust stdlib),
    in the default build. It does this by putting the error-reporting bits
    (miette crate) under an optional feature, and the logging (log crate) under
    a feature-controlled macro, and manually writing an Error impl rather than
    using thiserror. This completely avoids proc macros and the syn build slowness.

    The user can still get nice errors out of miette: this is enabled by specifying
    a Cargo feature --features isle-errors.

  2. To allow the user to optionally inspect the generated source, which nominally
    lives in a hard-to-find path inside target/ now, this PR adds a feature isle-in-source-tree
    that, as implied by the name, moves the target for ISLE generated source into
    the source tree, at cranelift/codegen/isle_generated_source/. It seems reasonable
    to do this when an explicit feature (opt-in) is specified because this is how ISLE regeneration
    currently works as well. To prevent surprises, if the feature is not specified, the
    build fails if this directory exists.

The compile-time measures as follows on my system: a clean build (cargo clean first
then cargo build -p cranelift-codegen) on a MacBook M1 Air (macOS/aarch64) gives:

Before:

Finished dev [unoptimized + debuginfo] target(s) in 8.26s
cargo build -p cranelift-codegen  22.29s user 2.05s system 291% cpu 8.348 total

After:

Finished dev [unoptimized + debuginfo] target(s) in 9.96s
cargo build -p cranelift-codegen  28.08s user 2.43s system 303% cpu 10.055 total

so about 1.7 seconds slower.

With a default configuration, an ISLE error looks like this during build:

  --- stderr
  Error building ISLE files: ISLE errors:

  parse error: Unexpected token Symbol("asdf")

  To see a more detailed error report, run:

      $ cargo check -p cranelift-codegen --features isle-errors

When specifying the feature, one instead gets

  --- stderr
  Error building ISLE files:
    × parse error: Unexpected token Symbol("asdf")
     ╭─[src/isa/aarch64/inst.isle:1:1]
   1  asdf
     · 
     · ╰── Unexpected token Symbol("asdf")
   2 
     ╰────

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:02):

cfallin requested fitzgen for a review on PR #4143.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:02):

cfallin requested alexcrichton for a review on PR #4143.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:05):

cfallin edited PR #4143 from isle-no-checked-in-code to main:

This PR fixes #4066: it modifies the Cranelift build.rs workflow to
invoke the ISLE DSL compiler on every compilation, rather than only
when the user specifies a special "rebuild ISLE" feature.

The main benefit of this change is that it vastly simplifies the mental
model required of developers, and removes a bunch of failure modes
we have tried to work around in other ways. There is now just one
"source of truth", the ISLE source itself, in the repository, and so there
is no need to understand a special "rebuild" step and how to handle
merge errors. There is no special process needed to develop the compiler
when modifying the DSL. And there is no "noise" in the git history produced
by constantly-regenerated files.

The two main downsides we discussed in #4066 are:

This PR addresses each of these concerns:

  1. To maintain reasonable compile time, it includes work to cut down the
    dependencies of the cranelift-isle crate to nothing (only the Rust stdlib),
    in the default build. It does this by putting the error-reporting bits
    (miette crate) under an optional feature, and the logging (log crate) under
    a feature-controlled macro, and manually writing an Error impl rather than
    using thiserror. This completely avoids proc macros and the syn build slowness.

    The user can still get nice errors out of miette: this is enabled by specifying
    a Cargo feature --features isle-errors.

  2. To allow the user to optionally inspect the generated source, which nominally
    lives in a hard-to-find path inside target/ now, this PR adds a feature isle-in-source-tree
    that, as implied by the name, moves the target for ISLE generated source into
    the source tree, at cranelift/codegen/isle_generated_source/. It seems reasonable
    to do this when an explicit feature (opt-in) is specified because this is how ISLE regeneration
    currently works as well. To prevent surprises, if the feature is not specified, the
    build fails if this directory exists.

The compile-time measures as follows on my system: a clean build (cargo clean first
then cargo build -p cranelift-codegen) on a MacBook M1 Air (macOS/aarch64) gives:

Before:

Finished dev [unoptimized + debuginfo] target(s) in 8.26s
cargo build -p cranelift-codegen  22.29s user 2.05s system 291% cpu 8.348 total

After:

Finished dev [unoptimized + debuginfo] target(s) in 9.96s
cargo build -p cranelift-codegen  28.08s user 2.43s system 303% cpu 10.055 total

so about 1.7 seconds slower.

With a default configuration, an ISLE error looks like this during build:

  --- stderr
  Error building ISLE files: ISLE errors:

  parse error: Unexpected token Symbol("asdf")

  To see a more detailed error report, run:

      $ cargo check -p cranelift-codegen --features isle-errors

When specifying the feature, one instead gets

  --- stderr
  Error building ISLE files:
    × parse error: Unexpected token Symbol("asdf")
     ╭─[src/isa/aarch64/inst.isle:1:1]
   1  asdf
     · 
     · ╰── Unexpected token Symbol("asdf")
   2 
     ╰────

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 00:06):

cfallin updated PR #4143 from isle-no-checked-in-code to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:29):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:29):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:29):

abrown created PR review comment:

logging = ["log"]

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:52):

cfallin updated PR #4143 from isle-no-checked-in-code to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:52):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 03:52):

cfallin created PR review comment:

Fixed, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 04:26):

cfallin updated PR #4143 from isle-no-checked-in-code to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 05:25):

cfallin merged PR #4143.


Last updated: Dec 23 2024 at 12:05 UTC