Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace manual use of index newtypes bundles[bundle.index()] with Index impls and typed Vec wrappers #62

Open
cfallin opened this issue Jun 27, 2022 · 10 comments

Comments

@cfallin
Copy link
Member

cfallin commented Jun 27, 2022

Right now, regalloc2 uses an entity-component-system sort of pattern with toplevel Vecs of LiveBundle, VRegData, and the like, and newtype'd index wrappers like LiveBundleIndex, VRegIndex, etc. We have a whole bunch of instances of self.bundles[bundle.index()]....

Ideally we would make bundles a Vec-wrapper type that has an Index implementation that natively takes LiveBundleIndex, and then we could make all of these sites slightly less verbose.

@Amanieu
Copy link
Contributor

Amanieu commented Jun 28, 2022

Could we just use cranelift-entity for this?

@bjorn3
Copy link
Contributor

bjorn3 commented Jun 28, 2022

That would likely require moving cranelift-entity out of the wasmtime repo and independently versioning it. Otherwise you are pretty much stuck with two copies in case you are compiling cranelift.

@cfallin
Copy link
Member Author

cfallin commented Jun 28, 2022

Yeah I'd be hesitant to introduce a circular dependency in general between repositories. (Perhaps cranelift-entity really should be its own little independent library, but that's a slightly bigger question...)

@fitzgen
Copy link
Member

fitzgen commented Jul 5, 2022

Or regalloc2 should be under cranelift/ in the Wasmtime repo.

@cfallin
Copy link
Member Author

cfallin commented Jul 5, 2022

That's a much bigger discussion as well... at the time at least, we had good reasons to not do that: different (lighter-weight) CI here, historical precedent wrt regalloc.rs, general modularity (I would argue by default separable libraries should be separate, and the burden of argument is on the consolidation side, but that's of course subjective...). I'm not completely opposed to it now but that's a big thing to re-consider especially now that it's established, others have forked and contributed, may have direct references to it. Basically subsuming the whole repo into another one feels out of proportion to the upside "can use a nice indexing type". (Of course if anyone feels strongly about this they're welcome to create a toplevel issue for it!)

@Amanieu
Copy link
Contributor

Amanieu commented Sep 6, 2022

That would likely require moving cranelift-entity out of the wasmtime repo and independently versioning it. Otherwise you are pretty much stuck with two copies in case you are compiling cranelift.

Is this resolved by the upcoming Wasmtime 1.0 release?

FWIW I'm already using cranelift-entity outside of Cranelift, for my own compiler.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 6, 2022

If regalloc2 were to use cranelift-entity as is, you would get two copies of cranelift-entity. One used by regalloc2 from crates.io and one used by cranelift part of this repo.

@Amanieu
Copy link
Contributor

Amanieu commented Sep 7, 2022

Not if cranelift-entity was 1.0, since the 2 uses would be semver-compatible and Cargo would satisfy both of them with a single dependency version.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 7, 2022

I don't think that works when one of the versions is from crates.io and the other is a path dependency.

@cfallin
Copy link
Member Author

cfallin commented Sep 7, 2022

The switch to 1.0-series versions (bumping major number at each release) don't change anything here, I think; the plan is still for each release to be a semver bump wrt the last one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants