Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .claude/skills/testing-hashql/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ For testing MIR transformation and analysis passes directly with programmaticall
- Edge cases requiring specific MIR structures hard to produce from source
- Benchmarking pass performance

**Key features:**

- Transform passes return `Changed` enum (`Yes`, `No`, `Unknown`) to indicate modifications
- Test harness captures and includes `Changed` value in snapshots for verification
- Snapshot format: before MIR β†’ `Changed: Yes/No/Unknown` separator β†’ after MIR

**Quick Example:**

```rust
Expand All @@ -201,10 +207,10 @@ builder
let body = builder.finish(0, TypeBuilder::synthetic(&env).integer());
```

πŸ“– **Full Guide:** [resources/mir-builder-guide.md](resources/mir-builder-guide.md)
πŸ“– **Full Guide:** [references/mir-builder-guide.md](references/mir-builder-guide.md)

## References

- [compiletest Guide](resources/compiletest-guide.md) - Detailed UI test documentation
- [Testing Strategies](resources/testing-strategies.md) - Choosing the right approach
- [MIR Builder Guide](resources/mir-builder-guide.md) - Programmatic MIR construction for tests
- [compiletest Guide](references/compiletest-guide.md) - Detailed UI test documentation
- [Testing Strategies](references/testing-strategies.md) - Choosing the right approach
- [MIR Builder Guide](references/mir-builder-guide.md) - Programmatic MIR construction for tests
21 changes: 13 additions & 8 deletions .claude/skills/testing-hashql/references/mir-builder-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,11 @@ builder.build_block(bb_merge).ret(x); // x receives value from param

## Test Harness Pattern

Standard pattern used across transform pass tests:
Standard pattern used across transform pass tests. The harness captures and displays
the `Changed` return value to verify pass behavior:

```rust
use std::path::PathBuf;
use std::{io::Write as _, path::PathBuf};
use bstr::ByteVec as _;
use hashql_core::{
pretty::Formatter,
Expand Down Expand Up @@ -274,12 +275,16 @@ fn assert_pass<'heap>(
.format(DefIdSlice::from_raw(&bodies), &[])
.expect("should be able to write bodies");

text_format
.writer
.extend(b"\n\n------------------------------------\n\n");

// Run the pass
YourPass::new().run(context, &mut bodies[0]);
// Run the pass and capture change status
let changed = YourPass::new().run(context, &mut bodies[0]);

// Include Changed value in snapshot for verification
write!(
text_format.writer,
"\n\n{:=^50}\n\n",
format!(" Changed: {changed:?} ")
)
.expect("infallible");

// Format after
text_format
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use hashql_mir::{
context::MirContext,
def::{DefId, DefIdSlice, DefIdVec},
intern::Interner,
pass::{TransformPass as _, transform::CfgSimplify},
pass::{Changed, TransformPass as _, transform::CfgSimplify},
};

use super::{RunContext, Suite, SuiteDiagnostic, common::process_issues, mir_reify::mir_reify};
Expand Down Expand Up @@ -63,7 +63,7 @@ pub(crate) fn mir_pass_transform_cfg_simplify<'heap>(

let mut pass = CfgSimplify::new_in(&mut scratch);
for body in bodies.as_mut_slice() {
pass.run(&mut context, body);
let _: Changed = pass.run(&mut context, body);
}

process_issues(diagnostics, context.diagnostics)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use hashql_mir::{
context::MirContext,
def::{DefId, DefIdSlice, DefIdVec},
intern::Interner,
pass::{TransformPass as _, transform::DeadStoreElimination},
pass::{Changed, TransformPass as _, transform::DeadStoreElimination},
};

use super::{
Expand Down Expand Up @@ -45,7 +45,7 @@ pub(crate) fn mir_pass_transform_dse<'heap>(
// CFG -> SROA -> Inst -> DSE
let mut pass = DeadStoreElimination::new_in(&mut scratch);
for body in bodies.as_mut_slice() {
pass.run(&mut context, body);
let _: Changed = pass.run(&mut context, body);
}

process_issues(diagnostics, context.diagnostics)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use hashql_mir::{
context::MirContext,
def::{DefId, DefIdSlice, DefIdVec},
intern::Interner,
pass::{TransformPass as _, transform::InstSimplify},
pass::{Changed, TransformPass as _, transform::InstSimplify},
};

use super::{
Expand Down Expand Up @@ -44,7 +44,7 @@ pub(crate) fn mir_pass_transform_inst_simplify<'heap>(

let mut pass = InstSimplify::new_in(&mut scratch);
for body in bodies.as_mut_slice() {
pass.run(&mut context, body);
let _: Changed = pass.run(&mut context, body);
}

process_issues(diagnostics, context.diagnostics)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use hashql_mir::{
context::MirContext,
def::{DefId, DefIdSlice, DefIdVec},
intern::Interner,
pass::{TransformPass as _, transform::Sroa},
pass::{Changed, TransformPass as _, transform::Sroa},
};

use super::{
Expand Down Expand Up @@ -44,7 +44,7 @@ pub(crate) fn mir_pass_transform_sroa<'heap>(

let mut pass = Sroa::new_in(&mut scratch);
for body in bodies.as_mut_slice() {
pass.run(&mut context, body);
let _: Changed = pass.run(&mut context, body);
}

process_issues(diagnostics, context.diagnostics)?;
Expand Down
49 changes: 32 additions & 17 deletions libs/@local/hashql/mir/benches/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
clippy::similar_names
)]

use core::hint::black_box;
use core::{cmp, hint::black_box};

use codspeed_criterion_compat::{BatchSize, Bencher, Criterion, criterion_group, criterion_main};
use hashql_core::{
Expand Down Expand Up @@ -301,10 +301,10 @@ fn create_complex_cfg<'heap>(env: &Environment<'heap>, interner: &Interner<'heap

#[expect(unsafe_code)]
#[inline]
fn run_bencher(
fn run_bencher<T>(
bencher: &mut Bencher,
body: for<'heap> fn(&Environment<'heap>, &Interner<'heap>) -> Body<'heap>,
mut func: impl for<'env, 'heap> FnMut(&mut MirContext<'env, 'heap>, &mut Body<'heap>),
mut func: impl for<'env, 'heap> FnMut(&mut MirContext<'env, 'heap>, &mut Body<'heap>) -> T,
) {
// NOTE: `heap` must not be moved or reassigned; `heap_ptr` assumes its address is stable
// for the entire duration of this function.
Expand Down Expand Up @@ -348,8 +348,8 @@ fn run_bencher(
diagnostics: DiagnosticIssues::new(),
};

func(black_box(&mut context), black_box(body));
context.diagnostics
let value = func(black_box(&mut context), black_box(body));
(context.diagnostics, value)
},
BatchSize::PerIteration,
);
Expand Down Expand Up @@ -440,30 +440,45 @@ fn pipeline(criterion: &mut Criterion) {
let mut scratch = Scratch::new();

run_bencher(bencher, create_linear_cfg, |context, body| {
CfgSimplify::new_in(&mut scratch).run(context, body);
Sroa::new_in(&mut scratch).run(context, body);
InstSimplify::new().run(context, body);
DeadStoreElimination::new_in(&mut scratch).run(context, body);
let mut changed = CfgSimplify::new_in(&mut scratch).run(context, body);
changed = cmp::max(changed, Sroa::new_in(&mut scratch).run(context, body));
changed = cmp::max(changed, InstSimplify::new().run(context, body));
changed = cmp::max(
changed,
DeadStoreElimination::new_in(&mut scratch).run(context, body),
);

changed
});
});
group.bench_function("diamond", |bencher| {
let mut scratch = Scratch::new();

run_bencher(bencher, create_diamond_cfg, |context, body| {
CfgSimplify::new_in(&mut scratch).run(context, body);
Sroa::new_in(&mut scratch).run(context, body);
InstSimplify::new().run(context, body);
DeadStoreElimination::new_in(&mut scratch).run(context, body);
let mut changed = CfgSimplify::new_in(&mut scratch).run(context, body);
changed = cmp::max(changed, Sroa::new_in(&mut scratch).run(context, body));
changed = cmp::max(changed, InstSimplify::new().run(context, body));
changed = cmp::max(
changed,
DeadStoreElimination::new_in(&mut scratch).run(context, body),
);

changed
});
});
group.bench_function("complex", |bencher| {
let mut scratch = Scratch::new();

run_bencher(bencher, create_complex_cfg, |context, body| {
CfgSimplify::new_in(&mut scratch).run(context, body);
Sroa::new_in(&mut scratch).run(context, body);
InstSimplify::new().run(context, body);
DeadStoreElimination::new_in(&mut scratch).run(context, body);
let mut changed = CfgSimplify::new_in(&mut scratch).run(context, body);
changed = cmp::max(changed, Sroa::new_in(&mut scratch).run(context, body));
changed = cmp::max(changed, InstSimplify::new().run(context, body));
changed = cmp::max(
changed,
DeadStoreElimination::new_in(&mut scratch).run(context, body),
);

changed
});
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl<'env, 'heap, A: Allocator + Clone> AnalysisPass<'env, 'heap>

graph.derive(&body.local_decls, |id, _| id);

let Ok(()) = DataDependencyAnalysisVisitor {
Ok(()) = DataDependencyAnalysisVisitor {
graph: &mut graph,
constant_bindings: &mut constant_bindings,
context,
Expand Down
51 changes: 50 additions & 1 deletion libs/@local/hashql/mir/src/pass/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,55 @@ const fn simplify_type_name(name: &'static str) -> &'static str {
}
}

/// Indicates whether a pass modified the MIR.
///
/// Passes return this to signal whether they made changes, enabling the pass manager to skip
/// dependent re-analyses when nothing changed.
#[must_use]
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum Changed {
/// The pass definitely made modifications.
Yes = 2,
/// The pass may have made modifications, but precise tracking was not possible.
Unknown = 1,
/// The pass made no modifications.
No = 0,
}

impl Changed {
/// Returns `true` if the MIR may have changed.
///
/// This is the conservative choice: returns `true` for both [`Changed::Yes`] and
/// [`Changed::Unknown`], ensuring dependent analyses are invalidated when in doubt, but may
/// result in unnecessary passes to be run.
#[must_use]
pub const fn conservative(self) -> bool {
match self {
Self::Yes | Self::Unknown => true,
Self::No => false,
}
}

/// Returns `true` only if the MIR definitely changed.
///
/// This is the optimistic choice: returns `true` only for [`Changed::Yes`], assuming
/// [`Changed::Unknown`] did not actually modify anything. Use with caution, as this may
/// skip necessary re-runs of passes.
#[must_use]
pub const fn optimistic(self) -> bool {
match self {
Self::Yes => true,
Self::Unknown | Self::No => false,
}
}
}

impl From<bool> for Changed {
fn from(value: bool) -> Self {
if value { Self::Yes } else { Self::No }
}
}

/// A transformation or analysis pass over MIR.
///
/// Passes operate on a [`Body`] within a [`MirContext`], allowing them to read and modify the
Expand Down Expand Up @@ -86,7 +135,7 @@ pub trait TransformPass<'env, 'heap> {
///
/// The `context` provides access to the heap allocator, type environment, interner, and
/// diagnostic collection. The `body` can be read and modified in place.
fn run(&mut self, context: &mut MirContext<'env, 'heap>, body: &mut Body<'heap>);
fn run(&mut self, context: &mut MirContext<'env, 'heap>, body: &mut Body<'heap>) -> Changed;

/// Returns a human-readable name for this pass.
///
Expand Down
21 changes: 17 additions & 4 deletions libs/@local/hashql/mir/src/pass/transform/cfg_simplify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ use crate::{
terminator::{Goto, Terminator, TerminatorKind},
},
context::MirContext,
pass::TransformPass,
pass::{Changed, TransformPass},
};

/// Control-flow graph simplification pass.
Expand Down Expand Up @@ -479,8 +479,9 @@ impl<'env, 'heap, A: BumpAllocator> TransformPass<'env, 'heap> for CfgSimplify<A
/// Uses a worklist algorithm that processes blocks in reverse postorder and re-enqueues
/// predecessors when changes occur. This ensures optimization opportunities propagate
/// backward through the CFG until a fixed point is reached.
fn run(&mut self, context: &mut MirContext<'env, 'heap>, body: &mut Body<'heap>) {
fn run(&mut self, context: &mut MirContext<'env, 'heap>, body: &mut Body<'heap>) -> Changed {
let mut queue = WorkQueue::new(body.basic_blocks.len());
let mut changed = false;

// Process in reverse of reverse-postorder (i.e., roughly postorder) to handle
// successors before predecessors, maximizing optimization opportunities.
Expand All @@ -500,6 +501,7 @@ impl<'env, 'heap, A: BumpAllocator> TransformPass<'env, 'heap> for CfgSimplify<A
simplified = true;
}

changed |= simplified;
if !simplified {
continue;
}
Expand All @@ -510,11 +512,22 @@ impl<'env, 'heap, A: BumpAllocator> TransformPass<'env, 'heap> for CfgSimplify<A
}
}

if !changed {
// We haven't made any changes, meaning that we can skip further analysis, as no blocks
// will be made unreachable, and SSA won't be violated.
return Changed::No;
}

// Unreachable blocks will be dead, therefore must be removed
let mut dbe = DeadBlockElimination::new_in(&mut self.alloc);
dbe.run(context, body);
let _: Changed = dbe.run(context, body);

// Simplifications may break SSA (e.g., merged blocks with conflicting definitions).
SsaRepair::new_in(&mut self.alloc).run(context, body);
let _: Changed = SsaRepair::new_in(&mut self.alloc).run(context, body);

// We ignore the changed of the sub-passes above, because we **know** that we already
// modified, if they don't doesn't matter.

changed.into()
}
}
14 changes: 8 additions & 6 deletions libs/@local/hashql/mir/src/pass/transform/cfg_simplify/tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::path::PathBuf;
use std::{io::Write as _, path::PathBuf};

use bstr::ByteVec as _;
use hashql_core::{
Expand Down Expand Up @@ -44,11 +44,13 @@ fn assert_cfg_simplify_pass<'heap>(
.format(DefIdSlice::from_raw(&bodies), &[])
.expect("should be able to write bodies");

text_format
.writer
.extend(b"\n\n------------------------------------\n\n");

CfgSimplify::new().run(context, &mut bodies[0]);
let changed = CfgSimplify::new().run(context, &mut bodies[0]);
write!(
text_format.writer,
"\n\n{:=^50}\n\n",
format!(" Changed: {changed:?} ")
)
.expect("infallible");

text_format
.format(DefIdSlice::from_raw(&bodies), &[])
Expand Down
Loading
Loading