Anthony Bullard (Dec 05 2024 at 20:48): Anthony Bullard (Dec 05 2024 at 20:48):Getting some interesting output after a simple change in my Day 5, Part 2 solution for AOC:
😱 LLVM errors when defining module; I wrote the full LLVM IR to "main.ll"
Call parameter type does not match function signature!
%load_opaque2 = load { %list.RocList, %list.RocList, i64, float, i8 }, ptr %"#arg1", align 8, !dbg !1891
ptr %call_user_defined_compare_function = call fastcc i8 @"#UserApp_67_14a58b912b778d42457329574d6284a94c25ccd4d26148cd2e979b6a93d4ec9"(i64 %load_opaque, i64 %load_opaque1, { %list.RocList, %list.RocList, i64, float, i8 } %load_opaque2), !dbg !1891
Location: crates/compiler/build/src/program.rs:283:9
I don't have LSP in my normal Editor (neovim), but when I open in Zed - which is using the stable LSP - it's all good. But when I build with latest I get the above.
Anthony Bullard (Dec 05 2024 at 20:49):Give me a sec and I'll push an update to my repo and paste a link
Anthony Bullard (Dec 05 2024 at 21:12):Here it is:
https://github.com/gamebox/aoc-2024/blob/main/day5/puzzle2/main.roc
Brendan Hansknecht (Dec 05 2024 at 22:23):Can you fully type pageSprtByRules and see what output you get?
Sam Mohr (Dec 05 2024 at 22:24):I'm working on a reduction right now, almost done
Anthony Bullard (Dec 05 2024 at 22:24):I did, and it just gave me a weird type annotation warning
Anthony Bullard (Dec 05 2024 at 22:24):I don't think it likes that it only returns two of the three tags in the union maybe?
Brendan Hansknecht (Dec 05 2024 at 22:26):Are you typing it with the full union?
Sam Mohr (Dec 05 2024 at 22:28):module []
sortByRules = \ruleMap ->
\_a, b ->
afters =
Dict.get ruleMap b
|> Result.withDefault []
if List.isEmpty afters then LT else GT
calculateMiddleTotal = \{} ->
sorter = sortByRules (Dict.empty {})
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
Fails with
An internal compiler expectation was broken.
This is definitely a compiler bug.
Please file an issue here: <https://github.com/roc-lang/roc/issues/new/choose>
Errors defining module:
Call parameter type does not match function signature!
%load_opaque2 = load { %list.RocList, %list.RocList, i64, float, i8 }, ptr %"#arg1", align 8, !dbg !752
ptr %call_user_defined_compare_function = call fastcc i8 @test_1_3_52aff1341cf42f5e6559a2cf028663f7bbbc7576ac1948fc58784a0613b79(%lis
t.RocList %load_opaque, %list.RocList %load_opaque1, { %list.RocList, %list.RocList, i64, float, i8 } %load_opaque2), !dbg !752
Uncomment things nearby to see more details. IR written to `"/var/folders/23/7wjdwv0n5t1b12fgvjmpc_fr0000gp/T/test.ll"`
Location: crates/repl_expect/src/run.rs:561:9
Nope, I left it with just a _
Anthony Bullard (Dec 05 2024 at 22:30):I kinda wish the List builtin module had an exported type alias for the comparison union
Anthony Bullard (Dec 05 2024 at 22:30):Like Comp or something
Anthony Bullard (Dec 05 2024 at 22:30):I've had similar issues with Bool as well
Brendan Hansknecht (Dec 06 2024 at 01:04):The bug will either be the captures or the union. Not sure which. Should be easy to test though
Brendan Hansknecht (Dec 06 2024 at 01:05):I assume the actual generated mono is bad and that is why the llvm ir is bad
Anthony Bullard (Dec 06 2024 at 01:59):Maybe I can instrument the mono coming in?
Brendan Hansknecht (Dec 06 2024 at 03:00):We have a flag have mono verify iteself
Brendan Hansknecht (Dec 06 2024 at 03:01):Requires running roc through cargo I think (maybe works with any debug build of the compiler)
Brendan Hansknecht (Dec 06 2024 at 03:02):ROC_CHECK_MONO_IR=1
Anthony Bullard (Dec 06 2024 at 03:03):Cool
Anthony Bullard (Dec 06 2024 at 03:19):I called it like this and no additional logging:
ROC_CHECK_MONO_IR=1 cargo run -- build ~/Development/aoc-roc-2024/day5/puzzle2/main.roc
Double checked and check_ir found no problems
Anthony Bullard (Dec 06 2024 at 03:39):Here's some source (Unrelated technically)
fixUpdate : RuleMap -> (Update -> Update)
fixUpdate = \ruleMap -> \update ->
List.sortWith update (pageSortByRules ruleMap)
IR after RESET_REUSE:
procedure : `#UserApp.fixUpdate` {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}
procedure = `#UserApp.fixUpdate` (`#UserApp.ruleMap`: {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}):
ret `#UserApp.ruleMap`;
BTW, I love this Roc/LLVMIR hybrid :smile:
Anthony Bullard (Dec 06 2024 at 03:41):WOW, that's strange.
The IR for the actual problem function:
procedure : `#UserApp.pageSortByRules` {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}
procedure = `#UserApp.pageSortByRules` (`#UserApp.ruleMap`: {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}):
ret `#UserApp.ruleMap`;
Which is exactly the same...
Anthony Bullard (Dec 06 2024 at 03:41):Is this some weird some of interning issue?
Anthony Bullard (Dec 06 2024 at 03:41):I'm going to have to learn a LOT to figure that out
Brendan Hansknecht (Dec 06 2024 at 05:13):That function body looks very wrong (and by that I mean nonexistant)
Anthony Bullard (Dec 06 2024 at 11:48):Yeah, gotta love functions becomes the Identity function random
Anthony Bullard (Dec 06 2024 at 11:49):And the types are wrong for one of them
Anthony Bullard (Dec 06 2024 at 11:53):Again the pageSortByRules function is defined as :
pageSortByRules : RuleMap -> (U64, U64 -> _)
pageSortByRules = \ruleMap -> \a, b ->
afters : List U64
afters =
Dict.get ruleMap b
|> Result.withDefault []
contains : Bool
contains = List.contains afters a
if contains then
LT
else
EQ
Moving some functions back to lambdas....
fixUpdates : RuleMap, List Update -> List Update
fixUpdates = \ruleMap, updates ->
List.map updates \update ->
List.sortWith update \a, b ->
afters =
Dict.get ruleMap b
|> Result.withDefault []
contains = List.contains afters a
if contains then
LT
else
EQ
Produces this mono after specialization
procedure : `#UserApp.fixUpdates` List List U64
procedure = `#UserApp.fixUpdates` (`#UserApp.ruleMap`: {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}, `#UserApp.updates`: List List U64):
let `#UserApp.116` : List List U64 = CallByName `List.map` `#UserApp.updates` `#UserApp.ruleMap`;
ret `#UserApp.116`;
Which to me seems wrong right from the call to List.map. I would expect a #UserMap.DD type procedure to be passed as the second argument.
Anthony Bullard (Dec 06 2024 at 12:32):I found a possible candidate
procedure : `#UserApp.66` List U64
procedure = `#UserApp.66` (`#UserApp.update`: List U64, `#UserApp.ruleMap`: {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}):
let `#UserApp.119` : List U64 = CallByName `List.sortWith` `#UserApp.update` `#UserApp.ruleMap`;
ret `#UserApp.119`;
And that call is also wrong
Anthony Bullard (Dec 06 2024 at 12:33):But maybe my brain isn't processing the type signatures right here.
Anthony Bullard (Dec 06 2024 at 12:34):I'm guessing that {List {U32, U32}, List {U64, List U64}, U64, Float32, U8} is actually the closure struct
Anthony Bullard (Dec 06 2024 at 14:15):Sam Mohr said:
module [] sortByRules = \ruleMap -> \_a, b -> afters = Dict.get ruleMap b |> Result.withDefault [] if List.isEmpty afters then LT else GT calculateMiddleTotal = \{} -> sorter = sortByRules (Dict.empty {}) fixed = List.sortWith [] sorter List.len fixed expect calculateMiddleTotal {} == 123Fails with
An internal compiler expectation was broken. This is definitely a compiler bug. Please file an issue here: <https://github.com/roc-lang/roc/issues/new/choose> Errors defining module: Call parameter type does not match function signature! %load_opaque2 = load { %list.RocList, %list.RocList, i64, float, i8 }, ptr %"#arg1", align 8, !dbg !752 ptr %call_user_defined_compare_function = call fastcc i8 @test_1_3_52aff1341cf42f5e6559a2cf028663f7bbbc7576ac1948fc58784a0613b79(%lis t.RocList %load_opaque, %list.RocList %load_opaque1, { %list.RocList, %list.RocList, i64, float, i8 } %load_opaque2), !dbg !752 Uncomment things nearby to see more details. IR written to `"/var/folders/23/7wjdwv0n5t1b12fgvjmpc_fr0000gp/T/test.ll"` Location: crates/repl_expect/src/run.rs:561:9
Don't know how you ran this, but I just tried and can't reproduce on stable or latest
Anthony Bullard (Dec 06 2024 at 14:19):If I actually use this module, I get a illegal Option unwrap on this line:
debug_assert!(unspecialized.is_empty());
Ok, I had to do one more thing than this, annotate that the sorter's return type is _
Anthony Bullard (Dec 06 2024 at 14:31):If I annotate it with the expected type, but keep the implementation the same, same error.
Anthony Bullard (Dec 06 2024 at 14:32):If I annotate it to return the exact Tag union it returns, a mismatch
Anthony Bullard (Dec 06 2024 at 14:32):If I annotate it to return the Tag union it returns but make it open, a mismatch
Anthony Bullard (Dec 06 2024 at 14:34):Actually, I could reproduce with yours. I just had a weird editor bug where it was renaming Test.roc to test.roc
Anthony Bullard (Dec 06 2024 at 14:36):If I need a function that returns [A, B C], and I get a function that returns just [A, B] that should not be a mismatch. Comparing tag unions should be something like Set.isSubset a b (Making that up)
Anthony Bullard (Dec 06 2024 at 14:37):And {A, B} is a subset of {A, B, C}
Anthony Bullard (Dec 06 2024 at 14:42):Interesting to note that running roc build --optimize causes a seg fault instead :eyes:
Anthony Bullard (Dec 06 2024 at 14:44):@Anton as a not-Rust-pro, how do you cause a seg fault in rust outside of unsafe? I'm not saying this isn't happening in an unsafe block, but just want to know if we have known edges were that can happen.
Anthony Bullard (Dec 06 2024 at 14:55):Ok, I just read it myself. OOB slice index, dangling reference, uninitialized variables, and even some panics
Anton (Dec 06 2024 at 14:58):Hmm segfaults in the compiler are quite rare, but yeah, do a debug build cargo build --bin roc and run valgrind ./target/debug/roc YOURCOMMAND and share the output here
Anton (Dec 06 2024 at 14:59):To be clear the segfault is happening during roc build --optimize? Not when running the produced binary?
Anthony Bullard (Dec 06 2024 at 15:11):Yes
Anthony Bullard (Dec 06 2024 at 15:12):And I've minimized it to a function returning a tag union that is a closure referencing specifically a Dict
Anthony Bullard (Dec 06 2024 at 15:18):A custom tag union doesn't do it, a list doesn't do it, a struct doesn't do it
Anton (Dec 06 2024 at 15:19):Valgrind may be able to give additional hints even though you already have the function
Anthony Bullard (Dec 06 2024 at 15:36):If memory serves, Valgrind can't run on arm64/MacOS on my version of the OS
Anton (Dec 06 2024 at 15:47):Ah yes, didn't know you were on macos, I can run it, is it just roc build --optimize on https://github.com/gamebox/aoc-2024/blob/main/day5/puzzle2/main.roc ?
Anthony Bullard (Dec 06 2024 at 16:14):Yes
Anthony Bullard (Dec 06 2024 at 16:14):Thank you
Brendan Hansknecht (Dec 06 2024 at 16:27):Anthony Bullard said:
If I need a function that returns [A, B C], and I get a function that returns just [A, B] that should not be a mismatch. Comparing tag unions should be something like Set.isSubset a b (Making that up)
Yeah, we made functions return open tag unions by default, so I believe this should work.
Anthony Bullard (Dec 06 2024 at 16:28):It gives a mismatch and specifies a union with the tag that is not in the return value
Brendan Hansknecht (Dec 06 2024 at 16:28):Oh, but it won't work cause you pass it to something as a lambda
Brendan Hansknecht (Dec 06 2024 at 16:28):I bet that unification doesn't understand the open tag return type and how to unify
Anthony Bullard (Dec 06 2024 at 16:28):?
Anthony Bullard (Dec 06 2024 at 16:29):That should be a simple unification
Brendan Hansknecht (Dec 06 2024 at 16:31):Probably. Just guessing it isn't implemented. Returning open tags was a feature added to roc later. It probably didn't consider lambdas when it was written. Probably just considered returned values from calls
Richard Feldman (Dec 06 2024 at 16:34):@Ayaz Hafiz might remember from back in the day :big_smile:
Anton (Dec 06 2024 at 16:34):I'm just getting "LLVM errors when defining module" no segfault, what's your roc commit @Anthony Bullard?
Anthony Bullard (Dec 06 2024 at 16:38):@Anton 0274d9b9971c91975c82a91c9e3057e6365f9701
Anthony Bullard (Dec 06 2024 at 16:40):That's with debug build
Anthony Bullard (Dec 06 2024 at 16:40):I can try with release if that makes a diff
Anthony Bullard (Dec 06 2024 at 16:41):It made no diff :smile:
Anthony Bullard (Dec 06 2024 at 16:42):I'm going to Linux-ify my old 2019 Mac Mini sometime soon, but right now, only have my M1 Pro MacBook Pro
Anthony Bullard (Dec 06 2024 at 16:45):I'm only going so deep on this right now because it's the second time in 5 days of AOC I've hit a compiler crash when doing something rather mundane in my solution - and couldn't overcome it without using a completely different solution. I appreciate all of your help @Brendan Hansknecht @Anton
Anton (Dec 06 2024 at 16:46):I still did not get a segfault on that commit but I'm running it with valgrind now anyway, it's taking a long time but that's expected
Anthony Bullard (Dec 06 2024 at 16:47):Interesting. I get it every time I hit this bug
Anthony Bullard (Dec 06 2024 at 16:48):Running this:
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : List U64
rulesMap = []
sorter = \_a, _b -> if List.isEmpty rulesMap then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
Results in no error with build --optimize
Anthony Bullard (Dec 06 2024 at 16:49):But this does
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : Dict U64 (List Str)
rulesMap = Dict.empty {}
sorter = \_a, _b -> if Dict.isEmpty rulesMap then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
Sorry, a segfault
Anthony Bullard (Dec 06 2024 at 16:50):And most interestingly to me, this also doesn't segfault and compiles as expected:
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : Dict U64 (List Str)
rulesMap = Dict.empty {}
sorter = \a, b -> if a > b then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
The only difference is we don't close over the Dict
Anthony Bullard (Dec 06 2024 at 16:53):This DOES fail with a segfault (using Result * *)
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : Result U64 [BooHoo]
rulesMap = Err BooHoo
sorter = \_a, _b -> if Result.isOk rulesMap then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
Also fails with segfault when using Set:
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : Set U64
rulesMap = Set.empty {}
sorter = \_a, _b -> if Set.isEmpty rulesMap then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
Jesus, it even fails with just closing over a Str
Anthony Bullard (Dec 06 2024 at 16:58):Supplying the same function (in any permutation) to something like List.map is no issue
Anthony Bullard (Dec 06 2024 at 16:59):And it always seems happy with closing over (List *)
Anton (Dec 06 2024 at 17:00):Jesus, it even fails with just closing over a
Str
That is nice actually, it simplifies the bug no?
Ayaz Hafiz (Dec 06 2024 at 17:01):can you share the example with Str?
Anthony Bullard (Dec 06 2024 at 17:01):But it works with just U64 @Anton
Anthony Bullard (Dec 06 2024 at 17:02):@Ayaz Hafiz as you please:
module [calculateMiddleTotal]
calculateMiddleTotal = \{} ->
rulesMap : Str
rulesMap = "Hello"
sorter = \_a, _b -> if Str.isEmpty rulesMap then GT else LT
fixed = List.sortWith [] sorter
List.len fixed
expect
calculateMiddleTotal {} == 123
I'm literally just changing three things, the type annotation of rulesMap, the value of it, and the condition in the if inside of the lambda to match that type
Ayaz Hafiz (Dec 06 2024 at 17:03):ty, taking a look
Anthony Bullard (Dec 06 2024 at 17:04):@Ayaz Hafiz Here's the mono for it, very interesting:
procedure : `Test.calculateMiddleTotal` U64
procedure = `Test.calculateMiddleTotal` (`Test.7`: {}):
let `Test.rulesMap` : Str = "Hello";
let `Test.12` : List [] = Array [];
let `Test.14` : Str = "UnresolvedTypeVar: specialize_symbol res_layout";
Crash `Test.14`
lol
Anthony Bullard (Dec 06 2024 at 17:04):I'm assuming that's an Easter Egg for a unexpected condition?
Ayaz Hafiz (Dec 06 2024 at 17:05):i have no idea, probably
Anthony Bullard (Dec 06 2024 at 17:05):Remember, this is supposed to be a minimal repro for the problem
Ayaz Hafiz (Dec 06 2024 at 17:05):yep
Anthony Bullard (Dec 06 2024 at 17:05):And when running against the real code, the lambda body was instead becoming the id function
Anthony Bullard (Dec 06 2024 at 17:06):Here's the mono for that
procedure : `#UserApp.fixUpdate` {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}
procedure = `#UserApp.fixUpdate` (`#UserApp.ruleMap`: {List {U32, U32}, List {U64, List U64}, U64, Float32, U8}):
ret `#UserApp.ruleMap`;
From this source:
fixUpdate : RuleMap -> (Update -> Update)
fixUpdate = \ruleMap -> \update ->
List.sortWith update (pageSortByRules ruleMap)
My eyes still don't feel trained to read the mono ir very well, but that definitely looks like id to me
Anthony Bullard (Dec 06 2024 at 17:08):It has something to do with the layout of the closed over value I'm assuming
Anthony Bullard (Dec 06 2024 at 17:09):I don't even know yet what to instrument to find the issue. But I'm trying to learn :-)
Ayaz Hafiz (Dec 06 2024 at 17:09):Anthony Bullard said:
My eyes still don't feel trained to read the mono ir very well, but that definitely looks like
idto me
For fixUpdate you mean?
Anthony Bullard (Dec 06 2024 at 17:11):Yeah, but it looks like it takes an arg #UserApp.ruleMap of the type {List {U32, U32}, List {U64, List U64}, U64, Float32, U8} and returns the same type, which is correct since it rets that arg as the only instruction.
Ayaz Hafiz (Dec 06 2024 at 17:12):yes that's fine. But it's not returning a function, it's returning the captures of the defunctionalized function
Anthony Bullard (Dec 06 2024 at 17:12):Ok, so that's the closure env?
Ayaz Hafiz (Dec 06 2024 at 17:13):yes, the env of the underlying \update -> ... fn
Anthony Bullard (Dec 06 2024 at 17:13):That's what I suspected
Ayaz Hafiz (Dec 06 2024 at 17:14):wait so for the List example you're seeing a crash when generating LLVM code right?
Ayaz Hafiz (Dec 06 2024 at 17:14):something like this
An internal compiler expectation was broken.
This is definitely a compiler bug.
Please file an issue here: <https://github.com/roc-lang/roc/issues/new/choose>
Errors defining module:
Call parameter type does not match function signature!
%load_opaque2 = load %str.RocStr, ptr %"#arg1", align 8, !dbg !491
ptr %call_user_defined_compare_function = call fastcc i8 @Test_sorter_e84248fb50d0833361d0417df114b0b3b3448fff97c39cdde963b09a9aebb8(ptr %load_opaque, ptr %load_opaque1, %str.RocStr %load_opaque2), !dbg !491
No List is the only thing that works
Ayaz Hafiz (Dec 06 2024 at 17:15):err sorry the Str one you posted earlier
Ayaz Hafiz (Dec 06 2024 at 17:15): Anthony Bullard (Dec 06 2024 at 17:16):Similar, yeah
An internal compiler expectation was broken.
This is definitely a compiler bug.
Please file an issue here: <https://github.com/roc-lang/roc/issues/new/choose>
😱 LLVM errors when defining module; I wrote the full LLVM IR to "main.ll"
Call parameter type does not match function signature!
%load_opaque2 = load %str.RocStr, ptr %"#arg1", align 8, !dbg !881
ptr %call_user_defined_compare_function = call fastcc i8 @Test_sorter_2c7d993eadf275d994a1f98b824972fece3cfca6b6ac52dd7bb717e1f5753(ptr %load_opaque, ptr %load_opaque1, %str.RocStr %load_opaque2), !dbg !881
Location: crates/compiler/build/src/program.rs:283:9
ok well the code generation is definitely wrong
Ayaz Hafiz (Dec 06 2024 at 17:29):ive forgotten how all this works lol
Ayaz Hafiz (Dec 06 2024 at 17:29):the code needs to be simplified a lot
Brendan Hansknecht (Dec 06 2024 at 17:31):Do you think the mono is wrong or the llvm code gen is wrong?
Anthony Bullard (Dec 06 2024 at 17:32):Mono is doing something wrong
Ayaz Hafiz (Dec 06 2024 at 17:32):no mono is fine
Ayaz Hafiz (Dec 06 2024 at 17:33):the llvm gen is producing the wrong types
Anthony Bullard (Dec 06 2024 at 17:33):This is how we get that llvm I showed the the Str value being closed over:
In mono/src/ir.rs (in the function specialize_symbol)
Some(partial_proc) => {
let arg_var = arg_var.unwrap_or(partial_proc.annotation);
// this symbol is a function, that is used by-name (e.g. as an argument to another
// function). Register it with the current variable, then create a function pointer
// to it in the IR.
let res_layout = return_on_layout_error!(
env,
layout_cache.raw_from_var(env.arena, arg_var, env.subs),
"specialize_symbol res_layout"
);
It's different for the other cases
Brendan Hansknecht (Dec 06 2024 at 17:47):That's for rooting it to llvm. I'll definitely take a look later today. I haven't followed everything, what is the minimal roc file to repro?
Anthony Bullard (Dec 06 2024 at 17:52):@Brendan Hansknecht These two have different, but similar issues:
https://roc.zulipchat.com/#narrow/channel/463736-bugs/topic/LLVM.20IR.20output.20in.20compiler.20crash.20during.20AOC!/near/486557861
https://roc.zulipchat.com/#narrow/channel/463736-bugs/topic/LLVM.20IR.20output.20in.20compiler.20crash.20during.20AOC!/near/486555486
Anton (Dec 06 2024 at 18:16):Update: valgrind did not report any errors
Anthony Bullard (Dec 06 2024 at 18:22)::thinking:
Paul Stanley (Dec 06 2024 at 18:38):FWIW I got more or less the same error as the one at the top of this thread doing yesterday. The context was List.sortWith and at first I thought it might be the rather specific function I had (which was a closure over a Dict) ... but replacing it even with the most tediously vanilla function produced the same issue. I ended up having to write my own mergesort rather than use List.sortWith. I realise that's horribly unspecific but I hadn't looked at this thread so didn't save the output, but just shrugged and moved on, so I don't have the code I was running. I do have the LLVM IR, if that's of any interest, but I have no idea what I'd be looking for!
Brendan Hansknecht (Dec 06 2024 at 18:51):Yeah, probably a bug with calling the zig builtin for sortWith
Anthony Bullard (Dec 06 2024 at 20:13):I want to test this case with other functions that expect a callback that returns a tag union
Anthony Bullard (Dec 06 2024 at 20:13):Outside of say Result
Anthony Bullard (Dec 06 2024 at 21:28):Ok, definitely seems like only List.sortWith
Anthony Bullard (Dec 06 2024 at 21:29):I just checked List.walkWith which has a callback which has a similar unnamed tag union and no issues with the closure over Dict k v
Anthony Bullard (Dec 06 2024 at 22:05):Damn, it's been so long since I worked with LLVM IR, I didn't even know about opaque pointers!
Anthony Bullard (Dec 06 2024 at 22:12):Ok, so we are loading a struct from a pointer and then supplying that as the third arg to the sorter, which is expecting three pointers. So the whole problem is SOMETHING thinks the layout allows it to be passed by value, but SOMETHING else is like "Nah, give me a pointer bro"
Anthony Bullard (Dec 06 2024 at 22:14):I think if the llvmir is this:
```llvm
%call_user_defined_compare_function = call fastcc i8 @Test_sorter_ebcdc7d352ecfa1e7d1b4ba0644f3ace5e7298b5a4113365f27eee831460e3a2(ptr %load_opaque, ptr %load_opaque1, ptr %"#arg1"), !dbg !924
Then it will work as expected, minus probably some other detail I'm missing
Can I just build the main.ll with llvm directly?
Luke Boswell (Dec 06 2024 at 22:18):Zig is pretty nice with llvm, zig build-exe -lc main.ll host-stub-things.zig you probably will need to provide an implementation for roc_alloc and friends in another zig file
Anthony Bullard (Dec 06 2024 at 22:20):@Luke Boswell Can I just give it an ll file and a platform .a file?
Luke Boswell (Dec 06 2024 at 22:20):Thats the easiest
Anthony Bullard (Dec 06 2024 at 22:20):I was trying llc and then clang
Anthony Bullard (Dec 06 2024 at 22:20):Let's see
Anthony Bullard (Dec 06 2024 at 22:20):Gotta find the platform cache on my system
Anthony Bullard (Dec 06 2024 at 22:21):I found it the other day...
Luke Boswell (Dec 06 2024 at 22:21):~/.cache/roc/...
Luke Boswell (Dec 06 2024 at 22:22):Im on my phone rn..
Anthony Bullard (Dec 06 2024 at 22:48):I found it, thanks Luke.
Anthony Bullard (Dec 06 2024 at 22:48):Ok, verified that we just to NOT load the struct from the pointer
Anthony Bullard (Dec 06 2024 at 22:48):Now time to figure out where in the hell we make that decision :-)
Luke Boswell (Dec 06 2024 at 22:49):gen_llvm?
Anthony Bullard (Dec 06 2024 at 22:49):Yeah, I got that part
Anthony Bullard (Dec 06 2024 at 22:51):Probably around is_passed_by_reference in layout.rs I'd assume
Anthony Bullard (Dec 06 2024 at 22:51):I'll do this goose chase later
Anthony Bullard (Dec 06 2024 at 22:52):But good to know that I'm making progress that others probably could have done in 30 minutes in instead 2 days
Anthony Bullard (Dec 06 2024 at 22:52)::-)
Luke Boswell (Dec 06 2024 at 22:52):Thanks for sharing the action... this is better than any Netflix series
Anthony Bullard (Dec 06 2024 at 22:52):I'm showing my thinking so that hopefully a more experienced dev on the team can help me avoid deadends and give me tips.
Anthony Bullard (Dec 06 2024 at 22:53):I came in knowing nothing about Roc's compiler
Anthony Bullard (Dec 07 2024 at 00:47):Ok, I definitely found where the issue manifests, hopefully will found the issue soon.
Brendan Hansknecht (Dec 07 2024 at 00:53):Yeah, you seem to be diving into this quite well
Brendan Hansknecht (Dec 07 2024 at 00:53):The fun of c abi and such
Anthony Bullard (Dec 07 2024 at 00:57):Yeah, I just can't figure out why the hell it looks at the roc function's type, and says "I see your llvm type is i8 (ptr, ptr, ptr), but I'm going to give you (ptr, ptr, { %list.RocList, %list.RocList, i64, float, i8 }) anyway"
Anthony Bullard (Dec 07 2024 at 01:02):LLVM decides the value type is a Struct instead of a value
Anthony Bullard (Dec 07 2024 at 01:06):So it seems that the function type is wrong
Anthony Bullard (Dec 07 2024 at 01:12):Shit....this is either coming from inkwell, or we are doing something weird when populating the environment
Luke Boswell (Dec 07 2024 at 01:13):I wonder if it's worth testing out this branch https://github.com/roc-lang/roc/pull/6921
Luke Boswell (Dec 07 2024 at 01:14):The LLVM upgrade is basically done... we just have some CI shenanigans to land that upgrade
Anthony Bullard (Dec 07 2024 at 01:14):Does it upgrade inkwell?
Luke Boswell (Dec 07 2024 at 01:14):Yes
Luke Boswell (Dec 07 2024 at 01:14):We are currently using an older custom fork, but this upgrades us to a more recent release
Brendan Hansknecht (Dec 07 2024 at 01:46):Reminder that llvm does not deal with abi
Brendan Hansknecht (Dec 07 2024 at 01:47):It leaves that up to us
Brendan Hansknecht (Dec 07 2024 at 01:47):It passes a pointer if we tell it to pass a pointer. It passes a struct if we tell it to pass a struct
Anthony Bullard (Dec 07 2024 at 01:47):Yes by the function type
Brendan Hansknecht (Dec 07 2024 at 01:48):So likely, this is a case of two different parts of our code disagreeing (that or zig generated llvm following cabi and us disagreeing)
Brendan Hansknecht (Dec 07 2024 at 01:48):Anthony Bullard said:
Yes by the function type
Not completely if I recall. I think we generate a call instruction without verifying the llvm function signature. It is solely generated off the roc mono types
Luke Boswell (Dec 07 2024 at 01:51):@Anthony Bullard ... I'm also interested in learning more about this aspect of our compiler. If you keep dropping thoughts in here as you go I would appreciate that. :smiley:
I've been poking at the compiler from both ends, but mono is still black magic
Anthony Bullard (Dec 07 2024 at 01:54):I have a lot to learn still
Ayaz Hafiz (Dec 07 2024 at 06:44):Anthony Bullard said:
I think if the llvmir is this:
```llvm
%call_user_defined_compare_function = call fastcc i8 @Test_sorter_ebcdc7d352ecfa1e7d1b4ba0644f3ace5e7298b5a4113365f27eee831460e3a2(ptr %load_opaque, ptr %load_opaque1, ptr %"#arg1"), !dbg !924Then it will work as expected, minus probably some other detail I'm missing ```` I think the problem is that `Test_sorter_ebcdc7d352ecfa1e7d1b4ba0644f3ace5e7298b5a4113365f27eee831460e3a2` is defined to take a ptr in the third parameter but it should be RocStr (ptr + 2 other fields)
Ayaz Hafiz (Dec 07 2024 at 06:50):oh wait no yeah the call site is wrong
Ayaz Hafiz (Dec 07 2024 at 06:50):although passing the str via reference seems kind of silly but that's a separate thing
Ayaz Hafiz (Dec 07 2024 at 07:02):Alright this is the fix for at least one of the issues
https://github.com/roc-lang/roc/pull/7317
Ayaz Hafiz (Dec 07 2024 at 07:02):Not sure why there's a unique path for this specifically but
Ayaz Hafiz (Dec 07 2024 at 07:04):seems to fix the Dict example too
Brendan Hansknecht (Dec 07 2024 at 07:11):Yeah, we have a lot of abi mess in the llvm backend that needs to be merged into one universal tagged abi. That just takes arguments with their type info and generates correct c abi. We have a lot of adhoc stuff today
Ayaz Hafiz (Dec 07 2024 at 07:27):this isn't ABI though. there should just be a better abstraction for loading parameters for internal calls.
Anthony Bullard (Dec 07 2024 at 11:22):@Ayaz Hafiz You are my personal hero right now. That change emits the exact llvm I was doing by hand, and fixes the issue in the minimal repro!
Anthony Bullard (Dec 07 2024 at 11:23):I am happy that I was hovering over this exact point in the code, but I had no idea what to do yet
Anthony Bullard (Dec 07 2024 at 11:25):And how that I see this, it makes a LOT of sense. The layout_interner can look out the closure data and tell us that it should be passed by reference or not. And then we only create the new load instruction of course when it is passed by value.
Anthony Bullard (Dec 07 2024 at 11:28):And it also fixes my AOC solution. Now for me to finish it :smile:
Ayaz Hafiz (Dec 07 2024 at 17:09):sweet!
Isaac Van Doren (Dec 07 2024 at 18:46):I ran into this issue in my solution and this fixed it for me too. Thanks!
Last updated: Jul 06 2025 at 12:14 UTC