[bug] Compiler stuck on building x86-linux target #308
Conversation
|
Okay so it's the register allocator going into an infinite loop here: virgil/aeneas/src/mach/LinearScanRegAlloc.v3 Lines 101 to 105 in be79dbd So what has happened is the I confirmed this by adding: to the for loop above. I get: P.S. @titzer Running $ V3C_OPTS="-dwarf=true -symbols=true" aeneas bootstrapgives me for the x86-64-linux target. |
|
Print the SSA and the SSA uid of the failing register says it's the
SSA dump of the
|
|
Okay. So @titzer the high-level reason for the bug is that the live range of the I'm pretty much at my limit of virgil internal debugging abilities. Hope this was useful. I would recommend creating an assertion library and adding assertions. This would have been very easy to detect if there was an assertion against creating a circular linked list. EDIT: Did a bit more digging and exactly what is happening is that virgil/aeneas/src/mach/LinearScanRegAlloc.v3 Line 264 in be79dbd Later on, when the reg allocator actually processes SSAInstr @70003 itself, it results in creating the circular list like mentioned in the above comment.
The quick fix, of course, is to just break from the infinite |
|
Hi @k-sareen I did a similar analysis and came to the conclusion that the test in question has a critical edge that the register allocator doesn't handle properly. One fix is to split all critical edges before codegen, but I don't like that, because the other targets and other allocators (other than the Wasm target, which needs to split critical edges for shadow stack spilling) don't need this. There is a branch https://github.com/titzer/virgil/tree/fix_lsra where I was tinkering with this. The real fix is to dive deeper into why the allocator doesn't handle critical edges, particularly a critical edge that points back to a loop header. |
Oh I didn't realize that. Apologies for the walls of text then haha.
I'm not sure what do you mean here. There is no loop in this particular case right. Or do you mean you've been able to reproduce a failing case with a loop. Also maybe it's the GC person in me talking, but surely one solution is to just avoid adding/processing something to the list that is already there? Maybe all we need is a bit in |
I'd like to understand why the variable in question has a nested live range like that and fix it in the allocator. One fix would be to have a counter instead of a bit, but I don't want to do that without understanding how the example gave rise to this. The examples I wrote in the branch where I was investigating ending up triggering a paranoid assertion, but I couldn't reproduce the infinite loop except in the original. I'm planning to do a stable release, so I'd like to fix this before that release. Any help would be appreciated. (I was working in this branch: https://github.com/titzer/virgil/tree/fix_lsra) |
|
Right. I'll try and look into it. It'd be great if we could have a MWE instead -- I'll see if I can trigger this bug with a smaller test case. |
|
EDIT: No actually. See LSRA pos 18 and 54 below: This is the nested live range. The ranges for These are the instructions: These instructions correspond to SsaInstrs: So correct me if I'm wrong, but what is happening then is that the Live interval range printProcessing broken block: 44678
liveness for block #44678 at bottom
XX... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... .....
liveness for block #44678 at top
X.... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... ..... .....
1: def def:@44677#0u0@edi
2: use use:@70002#2u8@eax
2: kill
2: live
3: def def:@70003#1u2@eax
4: use use:@70003#1u10@{gpr}
6: use use:@44677#0u18@edi use:@44680#5u20@eax
6: kill
6: live
7: def def:@70007#4u12@eax
12: end #4
12: use use:@70007#4u26
16: end #1
16: end #0
16: use use:@70008#9u28@eax
18: start #1
18: start #0
18: use use:@44677#0u36@edi
18: kill
18: live
19: def def:@70010#10u30@eax
20: end #10
20: use use:@70003#1u44@edi use:@70010#10u46@eax
20: kill
20: live
21: def def:@70012#11u38@eax
24: use use:@44677#0u54@{gpr}
25: def def:@70015#13u52@{gpr}
26: end #13
26: use use:@70015#13u56
30: use use:@44677#0u64@edi use:@44695#16u66@eax
30: kill
30: live
31: def def:@70017#15u58@eax
36: end #15
36: use use:@70017#15u72
40: use use:@44677#0u80@edi
40: kill
40: live
41: def def:@70019#19u74@eax
42: end #19
42: use use:@70003#1u88@edi use:@70019#19u90@eax
42: kill
42: live
43: def def:@70020#20u82@eax
46: use use:@44677#0u98@{gpr}
47: def def:@70022#22u96@{gpr}
48: end #22
48: use use:@70022#22u100
52: use use:@70008#9u102@eax
54: start #1
54: start #0
54: end #0
54: use use:@44677#0u110@edi use:@44702#26u112@eax
54: kill
54: live
55: def def:@70018#25u104@eax
60: end #25
60: use use:@70018#25u118
64: end #1
64: use use:@70008#9u120@eax
66: start #1
66: end #1
66: use use:@70003#1u128@edi
66: kill
66: live
67: def def:@70024#30u122@eax
68: use use:@70025#32u136@eax
68: kill
68: live
69: def def:@70026#31u130@eax
70: use use:@70026#31u138@{gpr}
72: end #30
72: end #31
72: use use:@70026#31u146@edi use:@70024#30u148@eax
72: kill
72: live
73: def def:@70030#34u140@eax
74: end #34
74: use use:@70030#34u150@eax
76: use use:@70008#9u152@eax
0: entry def:@44677#0u0@edi
1: =
2: |+ alloc def:@70003#1u2@eax live:@44677#0u4 kill:u6@{all} use:@70002#2u8@eax
3: | =
4: | + stored_i use:@70003#1u10@{gpr}
5: | |
6: + |+ call def:@70007#4u12@eax live:@70003#1u14 kill:u16@{all} use:@44677#0u18@edi use:@44680#5u20@eax
7: | | =
8: | | |
9: | | |
10: | | |
11: | | |
12: | | + cmp_i use:@70007#4u26
13: | |
14: | | br
15: | |
16: + ret use:@70008#9u28@eax
17:
18: + call def:@70010#10u30@eax live:@70002#2u32 kill:u34@{all} use:@44677#0u36@edi
19: | | =
20: | + + call def:@70012#11u38@eax live:@70005#3u40 kill:u42@{all} use:@70003#1u44@edi use:@70010#10u46@eax
21: | | =
22: | |
23: | |
24: + | loadub def:@70015#13u52@{gpr} use:@44677#0u54@{gpr}
25: | | =
26: | | + cmp_i use:@70015#13u56
27: | |
28: | | br
29: | |
30: + | + call def:@70017#15u58@eax live:@70007#4u60 kill:u62@{all} use:@44677#0u64@edi use:@44695#16u66@eax
31: | | =
32: | | |
33: | | |
34: | | |
35: | | |
36: | | + cmp_i use:@70017#15u72
37: | |
38: | | br
39: | |
40: + | call def:@70019#19u74@eax live:@44680#5u76 kill:u78@{all} use:@44677#0u80@edi
41: | | =
42: | + + call def:@70020#20u82@eax live:@44683#6u84 kill:u86@{all} use:@70003#1u88@edi use:@70019#19u90@eax
43: | | =
44: | |
45: | |
46: + | loadub def:@70022#22u96@{gpr} use:@44677#0u98@{gpr}
47: | | =
48: | | + cmp_i use:@70022#22u100
49: | |
50: | | br
51: | |
52: | | + ret use:@70008#9u102@eax
53: | |
54: + | + call def:@70018#25u104@eax live:@44686#7u106 kill:u108@{all} use:@44677#0u110@edi use:@44702#26u112@eax
55: | =
56: | |
57: | |
58: | |
59: | |
60: | + cmp_i use:@70018#25u118
61: |
62: | br
63: |
64: + ret use:@70008#9u120@eax
65:
66: + call def:@70024#30u122@eax live:@44688#8u124 kill:u126@{all} use:@70003#1u128@edi
67: =
68: |+ alloc def:@70026#31u130@eax live:@70008#9u132 kill:u134@{all} use:@70025#32u136@eax
69: | =
70: | + stored_i use:@70026#31u138@{gpr}
71: | |
72: + + call def:@70030#34u140@eax live:@70010#10u142 kill:u144@{all} use:@70026#31u146@edi use:@70024#30u148@eax
73: =
74: + ret use:@70030#34u150@eax
75:
76: + ret use:@70008#9u152@eax
77:
|
|
Surely LSRA pos 18 should say Also a quick point of clarification: is re-defining the same variable in SSA expected? I see, for example, that the generated SSA uses (or more precisely, defines) |
Obviously no, redefined variables in SSA should not be allowed. However in the machine representation, VREGs might be redefined, but I think that should only happen on the Wasm target when using the shadow stack spiller. There are some smaller tests cases in the branch I referenced before. I find it a lot easier to work on smaller examples. |
Yeah -- I'll try them out. I just felt like I was close to resolving it.
Right. That's what confused me. So why is Virgil re-defining Also another quick question Re: DWARF debug info. Does the DWARF debug info Virgil generates embed values of variables or is it just for debug symbols because I haven't been able to get it to print variable values/names in EDIT: I mean this: My understanding of |
Oh, I see why you could conclude that. Both of these are SSA constants and are arguments to the SSA call instructions. The trace output includes the |
|
Okay so in the end, I think it's a very subtle bug caused by an interaction of SSA for
|
The question essentially is what are the conditions under which we should not return early and we should insert the |
|
|
I've fixed the bug in my branch but it's a bit ugly and definitely can be cleaned up: https://github.com/k-sareen/virgil/tree/fix/lsra-ks. I can make a PR and clean up all the debugging code if you want. See #312 for more information. PS I'll kinda be AWOL until Tuesday IST. |
In certain edge cases, `finishLoop` would not insert `LsraEnd`s for active variables resulting in nested live ranges which manifested as an infinite loop in the register allocator (see discussion here [1]). This commit fixes this bug by keeping track of whether a variable will be made active in the future or not (i.e. if it will have a live range). If a variable will have a live range in the future, then `finishLoop` will make sure to insert `LsraEnd`s for them to avoid the above bug. [1]: titzer#308
Compiling the code in this PR halts the compiler. This was originally discovered when adding tests to the JsonParser in the standard library.
Reproduce:
cdintotest/liband runTEST_TARGETS=x86-linux ./test.bash.