refactor: remove OnceLock
#9992
Conversation
JonasKruckenberg
requested review from
fitzgen
and removed request for
a team
github-actions
bot
added
cranelift
| env | ||
| } | ||
| static DEFAULT_MACHINE_ENV: MachineEnv = { | ||
| debug_assert!(PINNED_REG == 21); // We assumed this below in hardcoded reg list. |
There was a problem hiding this comment.
This probably goes in the PINNED_MACHINE_ENV above, no? (It's load-bearing there that we get it right, because we excluded the pinned register from allocation; the default environment is used when pinning is disabled so doesn't care what the pinned reg is)
| @@ -48,10 +48,26 @@ pub fn first_user_vreg_index() -> usize { | |||
| pub struct Reg(VReg); | |||
|
|
|||
| impl Reg { | |||
| /// Create a register from a physical register | |||
| // FIXME(const-hack) remove in favor of `From` impl | |||
There was a problem hiding this comment.
Can you say more here -- I'm not that familiar with the current state of constification of everything; is there some feature this is waiting on to stabilize or...?
There was a problem hiding this comment.
well, the feature that would be waiting for is const associated functions in traits (therefore allowing From::from to be const) but that afaik is in "never type land". I can remove the note as it doesn't make much sense to hold our breath on it
There was a problem hiding this comment.
Yeah, I think that's best -- usually when we have a FIXME we want it to be actionable, or at least give sufficient context so that we know what we're waiting on (i.e.: if someone else walked up to this code tomorrow, would they know what they need in order to fix it?). It's fine to evaluate questions like this once new Rust features land in the future.
There was a problem hiding this comment.
the issue with constness is that theres no good way to detect constness workarounds after the fact (ie reevaluating code like this is tricky since it appears as just somewhat odd but regular code) which is the whole reason for the const-hack comments but usually you'd tie them to specific RFCs so yeah i agree getting rid of it is fine
There was a problem hiding this comment.
Relevant RFC fresh off the press: rust-lang/rfcs#3762
This PR is another attempt at #8489 (superseding it) this time by making the necessary changes in
regalloc2(see here) to allowMachineEnvto be created in const expressions and therefore placed into statics without the need for lazy initialization (as mentioned in this comment)This PR also makes the necessary changes in register creation functions to make them
const.While I agree with @cfallin that this is the "correct" move, it is slightly annoying that Rusts const limitation makes the static initializers so verbose where a
1..=16range iterator was used previously, any ideas here are welcome :/Edit: marked the PR as draft until the regalloc2 upstream PR is merged