removing `Nat` · compiler development

Stream: compiler development

Topic: removing `Nat`

Richard Feldman (Oct 21 2023 at 01:28):

I did some Nat removals and realized that the implementation Dict uses Nat extensively, including in the record that represents it

Richard Feldman (Oct 21 2023 at 01:30):

@Brendan Hansknecht if we made the record itself opaque, do you think replacing the remaining usages of Nat in there is feasible?

Richard Feldman (Oct 21 2023 at 01:31):

or should we explore some way of trying to keep Nat around behind the scenes, and only used by Dict.roc internally?

Brendan Hansknecht (Oct 21 2023 at 01:31):

I think the record is already opaque

Brendan Hansknecht (Oct 21 2023 at 01:32):

Should be fine to just change everything to u64 since that is what list indices will use.

Richard Feldman (Oct 21 2023 at 01:33):

er sorry, I mean opaque and implemented behind the scenes, like how List is

Richard Feldman (Oct 21 2023 at 01:33):

like for example, this: https://github.com/roc-lang/roc/blob/cd7a031d09fcd0a2f95cb2933b675028cd0b7730/crates/compiler/builtins/roc/Dict.roc#L92-L111 includes dataIndices : List Nat

Richard Feldman (Oct 21 2023 at 01:33):

if we change that to List U64, then Dicts will take up another 4B on the stack on 32-bit targets for no benefit

Brendan Hansknecht (Oct 21 2023 at 01:35):

Oh yeah, it's an index map so it stores nats....

Brendan Hansknecht (Oct 21 2023 at 01:35):

I mean long term we need to deal with hashing differently on 32 bit systems among other things

Brendan Hansknecht (Oct 21 2023 at 01:36):

I think it would be fine for now to just make this a 64bit dict implementation and add a to-do for 32bit.

Richard Feldman (Oct 21 2023 at 01:37):

works for me! :thumbs_up:

Brendan Hansknecht (Oct 21 2023 at 01:39):

Also, long term, I'm not 100% sure we want to keep dict in roc. We may want to move it to zig. I tried a few times to port the final optimizations over, but kept getting perf regressions. I think the fine details of the dict impl may be a tiny bit low level to happily map to roc, but maybe a few more builtins or simd stuff could fix that....not fully sure.