Skip to content

Commit

Permalink
[Calyx-utils] Overhaul GlobalPositionTable + a bunch of CI nonsense (
Browse files Browse the repository at this point in the history
…#2377)

A quick touch up of the `GlobalPositionTable` to avoid UB without
getting too fancy. Main changes are use of `boxcar::Vec` instead of the
standard library Vec, so that we can now have lock-free concurrent
appends and accesses. This fits our use-case since we never de-allocate
files & positions and frees us from all the issues associated with using
locks. Since we're not using locks, we also don't need any unsafe to
extend lifetimes, and since `boxcar::Vec` doesn't reallocate stored
elements, we don't need to box the stored stuff.

I also use `LazyLock` instead of `lazy_static!` which is easier to deal
with.

The end result is a global position table that can be accessed through
associated functions on the `GlobalPositionTable` type, rather than
methods on an instance. I also changed the `String`s stored by `File` to
`Box<str>`, because we don't ever mutate them.

---
In the process of this, I ran face first in CI issues. Turns out that
because some of the tests run in the docker container, they aren't
actually building using the version of rust in `rust-toolchain.toml` and
instead were using the version of rust from the container (1.76). This
broke things since `LazyLock` was only stabilized in 1.80. I had a real
fight with the CI because it turns out that using the standard actions
in the docker container is not a straightforward as one would like. I'll
skip a recount of the entire ordeal---peruse the commits to witness my
pain---but I resolved things and made these tests consistent with the
rest of CI by specifically pulling the toolchain file before running the
rust install action. The end result is that even the docker container
tests do respect the indicated rust version, and we should be able to
bump the version of rust used by _EVERYTHING_ (if this isn't true I may
scream) by updating the file.
  • Loading branch information
EclecticGriffin authored Dec 17, 2024
1 parent 1505e5d commit 37d90d9
Show file tree
Hide file tree
Showing 10 changed files with 107 additions and 104 deletions.
34 changes: 29 additions & 5 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Install Rust stable
- name: Install Rust toolchain
uses: actions-rust-lang/setup-rust-toolchain@v1
- name: Build calyx dev
run: cargo build
Expand Down Expand Up @@ -107,16 +107,20 @@ jobs:
git checkout -f $GITHUB_SHA
git clean -fd
- name: Checkout toolchain configuration
uses: actions/checkout@v4
with:
sparse-checkout: |
rust-toolchain.toml
sparse-checkout-cone-mode: false
- name: Install Rust toolchain
uses: actions-rust-lang/setup-rust-toolchain@v1
- name: Build
uses: actions-rs/cargo@v1
with:
command: build
args: --all --all-features --manifest-path /home/calyx/cider/Cargo.toml

- name: Set location of cider binary
run: |
fud c stages.interpreter.exec /home/calyx/target/debug/cider
- name: Runt tests
working-directory: /home/calyx/cider/tests
run: |
Expand Down Expand Up @@ -159,6 +163,16 @@ jobs:
git checkout -f $GITHUB_SHA
git clean -fd
- name: Checkout toolchain configuration
uses: actions/checkout@v4
with:
sparse-checkout: |
rust-toolchain.toml
sparse-checkout-cone-mode: false

- name: Install Rust toolchain
uses: actions-rust-lang/setup-rust-toolchain@v1

- name: Install calyx-py, MrXL, and queues
working-directory: /home/calyx
run: |
Expand Down Expand Up @@ -226,6 +240,16 @@ jobs:
git checkout -f $GITHUB_SHA
git clean -fd
- name: Checkout toolchain configuration
uses: actions/checkout@v4
with:
sparse-checkout: |
rust-toolchain.toml
sparse-checkout-cone-mode: false

- name: Install Rust toolchain
uses: actions-rust-lang/setup-rust-toolchain@v1

- name: Build
uses: actions-rs/cargo@v1
with:
Expand Down
8 changes: 7 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ categories = ["compilers"]
homepage = "https://calyxir.org"
edition = "2021"
version = "0.7.1"
rust-version = "1.67"
rust-version = "1.80"

[workspace.dependencies]
itertools = "0.11"
Expand Down
36 changes: 15 additions & 21 deletions calyx-frontend/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,17 @@ impl CalyxParser {
})?;
// Add a new file to the position table
let string_content = std::str::from_utf8(content)?.to_string();
let file = GlobalPositionTable::as_mut()
.add_file(path.to_string_lossy().to_string(), string_content);
let file = GlobalPositionTable::add_file(
path.to_string_lossy().to_string(),
string_content,
);
let user_data = UserData { file };
let gpos_ref = GlobalPositionTable::as_ref();
let content = gpos_ref.get_source(file).to_owned();
drop(gpos_ref);

let content = GlobalPositionTable::get_source(file);

// Parse the file
let inputs =
CalyxParser::parse_with_userdata(Rule::file, &content, user_data)
CalyxParser::parse_with_userdata(Rule::file, content, user_data)
.map_err(|e| e.with_path(&path.to_string_lossy()))
.map_err(|e| {
calyx_utils::Error::parse_error(e.variant.message())
Expand Down Expand Up @@ -96,20 +97,17 @@ impl CalyxParser {
))
})?;
// Save the input string to the position table
let file =
GlobalPositionTable::as_mut().add_file("<stdin>".to_string(), buf);
let file = GlobalPositionTable::add_file("<stdin>".to_string(), buf);
let user_data = UserData { file };
let gpos_ref = GlobalPositionTable::as_ref();
let content = gpos_ref.get_source(file).to_owned();
drop(gpos_ref);
let content = GlobalPositionTable::get_source(file);

// Parse the input
let inputs =
CalyxParser::parse_with_userdata(Rule::file, &content, user_data)
CalyxParser::parse_with_userdata(Rule::file, content, user_data)
.map_err(|e| {
calyx_utils::Error::parse_error(e.variant.message())
.with_pos(&Self::error_span(&e, file))
})?;
calyx_utils::Error::parse_error(e.variant.message())
.with_pos(&Self::error_span(&e, file))
})?;
let input = inputs.single().map_err(|e| {
calyx_utils::Error::parse_error(e.variant.message())
.with_pos(&Self::error_span(&e, file))
Expand All @@ -124,11 +122,7 @@ impl CalyxParser {
fn get_span(node: &Node) -> GPosIdx {
let ud = node.user_data();
let sp = node.as_span();
let pos = GlobalPositionTable::as_mut().add_pos(
ud.file,
sp.start(),
sp.end(),
);
let pos = GlobalPositionTable::add_pos(ud.file, sp.start(), sp.end());
GPosIdx(pos)
}

Expand All @@ -137,7 +131,7 @@ impl CalyxParser {
pest::error::InputLocation::Pos(off) => (off, off + 1),
pest::error::InputLocation::Span((start, end)) => (start, end),
};
let pos = GlobalPositionTable::as_mut().add_pos(file, start, end);
let pos = GlobalPositionTable::add_pos(file, start, end);
GPosIdx(pos)
}

Expand Down
2 changes: 1 addition & 1 deletion calyx-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ string-interner.workspace = true
itertools.workspace = true
petgraph.workspace = true
symbol_table = { version = "0.3", features = ["global"] }
lazy_static.workspace = true
boxcar = "0.2.7"
2 changes: 1 addition & 1 deletion calyx-utils/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ impl Error {
post_msg: None,
}
}
pub fn location(&self) -> (String, usize, usize) {
pub fn location(&self) -> (&str, usize, usize) {
self.pos.get_location()
}
pub fn message(&self) -> String {
Expand Down
4 changes: 1 addition & 3 deletions calyx-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,5 @@ pub use math::bits_needed_for;
pub use namegenerator::NameGenerator;
pub use out_file::OutputFile;
pub use pos_string::PosString;
pub use position::{
FileIdx, GPosIdx, GlobalPositionTable, PosIdx, PositionTable, WithPos,
};
pub use position::{FileIdx, GPosIdx, GlobalPositionTable, PosIdx, WithPos};
pub use weight_graph::{BoolIdx, Idx, WeightGraph};
Loading

0 comments on commit 37d90d9

Please sign in to comment.