Subscribe to Label Action

cc @cfallin, @fitzgen

@jameysharp I think this is more-or-less ready for review now!

(In fact, I just spent a few hours doing the latest rebase/merge, and these are fairly painful because this PR does a lot of code-motion to factor out ISLE commonalities for mid-end and backends, so I'd really prefer to try to get this merged before much more changes to avoid another rebase hell :-) )

I've updated the implementations of the various rewriting and elaboration algorithms to avoid explicit recursion, and I've factored out a generic "analysis" mechanism for the egraph crate to own; these were the main cleanups I wanted/needd to do before the prototype would be considered production-ready, I think. Regarding the rule-rewriting reentrancy issue, I opted to simply cap the recursion with a static limit (max of 5 chained rewrites) for now, as (i) we'll need such a limit anyway to protect against accidentally infinitely-(co)recursive rules, and (ii) it is about an order of magnitude simpler than unwinding the nested-immediate-rewrites into some sort of "pending node queue" and splitting the id-space, and probably more efficient in the common case.

In any case, let me know what you think -- I'm eager for feedback!

@alexcrichton it looks like the verify-publish CI job is failing here because for some reason it's looking for the (not-yet-published) cranelift-egraph crate on crates.io rather than in the workspace. I copied the magic incantations for workspace-inheritance from the other intra-repo deps but I'm not sure if I'm missing something -- it's surprising that this fails if all the other builds work fine?

tested using cranelift-tools with the following clif code:

target x86_64 

function u0:359(i64) -> i8 system_v {

    sig0 = (i64) -> i8, i8 system_v
    fn0 = colocated u0:521 sig0

    block0(v0: i64):
	v3, v4 = call fn0(v0)
	return v3
}

when calling

./target/debug/clif-util compile --set use_egraphs=false --target x86_64-unknown-linux-gnu example.clif

the code compiles without error.

when setting use_egraphs=true the following error is produced:

thread 'main' panicked at 'enode depends directly on multi-value result', cranelift/codegen/src/egraph/elaborate.rs:396:33

stack backtrace:
 0: rust_begin_unwind
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
 1: core::panicking::panic_fmt
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
 2: cranelift_codegen::egraph::elaborate::Elaborator::process_elab_stack::{{closure}}
           at ./cranelift/codegen/src/egraph/elaborate.rs:396:33
 3: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:301:13
 4: core::option::Option<T>::map
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:929:29
 5: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/iter/adapters/map.rs:103:9
 6: <smallvec::SmallVec<A> as core::iter::traits::collect::Extend<<A as smallvec::Array>::Item>>::extend
           at /home/user/.local/share/cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-1.8.0/src/lib.rs:1742:36
 7: <smallvec::SmallVec<A> as core::iter::traits::collect::FromIterator<<A as smallvec::Array>::Item>>::from_iter
           at /home/user/.local/share/cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-1.8.0/src/lib.rs:1727:9
 8: core::iter::traits::iterator::Iterator::collect
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/iter/traits/iterator.rs:1836:9
 9: cranelift_codegen::egraph::elaborate::Elaborator::process_elab_stack
           at ./cranelift/codegen/src/egraph/elaborate.rs:391:60
10: cranelift_codegen::egraph::elaborate::Elaborator::elaborate_eclass_use
           at ./cranelift/codegen/src/egraph/elaborate.rs:285:9
11: cranelift_codegen::egraph::elaborate::Elaborator::elaborate_block
           at ./cranelift/codegen/src/egraph/elaborate.rs:511:13
12: cranelift_codegen::egraph::elaborate::Elaborator::elaborate_domtree
           at ./cranelift/codegen/src/egraph/elaborate.rs:532:21
13: cranelift_codegen::egraph::elaborate::Elaborator::elaborate
           at ./cranelift/codegen/src/egraph/elaborate.rs:583:9
14: cranelift_codegen::egraph::FuncEGraph::elaborate
           at ./cranelift/codegen/src/egraph.rs:348:9
15: cranelift_codegen::context::Context::optimize
           at ./cranelift/codegen/src/context.rs:203:13
16: cranelift_codegen::context::Context::compile_stencil
           at ./cranelift/codegen/src/context.rs:149:13
17: cranelift_codegen::context::Context::compile
           at ./cranelift/codegen/src/context.rs:224:23
18: cranelift_codegen::context::Context::compile_and_emit
           at ./cranelift/codegen/src/context.rs:132:29
19: clif_util::compile::handle_module
           at ./cranelift/src/compile.rs:71:33
20: clif_util::compile::run
           at ./cranelift/src/compile.rs:46:9
21: clif_util::main
           at ./cranelift/src/clif-util.rs:106:33
22: core::ops::function::FnOnce::call_once
           at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

file reduced from clif of core produced when attempting to compile core with https://github.com/bjorn3/rustc_codegen_cranelift patched to use this PR

it's surprising that this fails if all the other builds work fine?

You'll need to edit this list to include automatic publishing of a new crate.

it's surprising that this fails if all the other builds work fine?

You'll need to edit this list to include automatic publishing of a new crate.

It’s actually already there, from the earlier PR that introduced the crate (line 28); this seems to be something new related to the workspaces change?

Ah I only checked the diff of this PR, the list also needs to be topologically sorted so if cranelift-codegen is picking up a new dep then the list needs to be reordered.

Ah, that makes sense, thanks!

Yeah, sorry, that's a bit of a mess now due to all the rebasing -- there was a good amount of manual cherrypicking of new definitions. I'll see if I can clean up the diff a bit (given that this is hopefully merging soon there won't be other work that'll invalidate this with more conflicts later!).

I've gone over the diff to the ISLE-side and Rust-side definitions of the prelude and ensured that the common bits, which are new files from git's point of view, now have contents that are strictly in the same order and exactly match the removed lines from the old (lowering-only) prelude. (Latest commit does some reordering to make this true.)

(This involved a "diff-of-diffs" workflow and was a little ugly; and I wish git had better ways of showing code provenance for cross-file moves; but in the end at least I'm confident no definition was inadvertently missed or reverted to an older version.)

Thanks so much for the detailed review -- it was really helpful!

So ends one long saga, and begins another (actually using this thing) :-)

Have you measured the performance impact of this PR with egraphs disabled? I see that ScopedHashMap is used by simple_gvn, which I was recently noticing takes a non-trivial amount of time. It looks to me like the ScopedHashMap changes should make some operations faster, so it could be an improvement, but it would be nice to check that it's not a regression.

I did earlier on, and testing again just now with SpiderMonkey.wasm, I see, uh... a 6% speedup?

I was curious about this so I've just compared the current merge-base from main (d68ca37) with this branch (ec8a1a9). I see a 1% improvement in compile time/CPU cycles/instructions retired on the bz2, pulldown-cmark, and spidermonkey benchmarks. Not 6% but definitely nice!