-
Notifications
You must be signed in to change notification settings - Fork 81
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
lcc: compiler hangs on complex expression #76
Comments
Simplification that still hangs the compiler:
|
We seem stuck in a never ending recursion:
|
This looks funny:
These vAC's are different symbol objects with the same name. The rightmost is created by mkreg(). The left one by genspill(). "Our" inset() treats them as different. (But so does the original LCC code before inset() was added.) The same hangup issue occurs with the shift operators:
Interestingly, these operators get the same treatment in gt1.c:
|
If I replace
with
then the weird 'vAC to vAC' messages are gone. But the compiler still enters an unbounded recursion :-( |
LCC gets in an infinite loop of spilling and reloading vAC. Initially it spills vAC at some point, which is fine and understandable. Later it reloads, but at that point vAC is allocated, so it wants to spill it again. etc (The message “inserting load from vAC to vAC” looks like a red herring we can ignore for now.) |
Yep. I attempted to address this by extending the RA's support for register sets to include partial sets as well as complete sets and ensuring that all nodes that require one operand in vAC require the other operand in anything besides vAC. My guess is that I missed a spot somewhere. It would be great to have a generalized solution for the infinite spill/reload cycle, though I'm not sure what that would be. I believe that the approach I described above will work, but it is more ad-hoc than I would like. |
It’s a nice brain teaser now that I understand what happens. It indeed begs for a simple generalised solution, but I don't "see" it (yet). |
Annotated simplified debug output
The above annotation is not quite right. Some speculation: the MUL needs two reloads (LHS and RHS), but the compiler tries to emit them in reverse order? |
What does the full tree look like post-canonicalization? |
It is called only at the beginning:
But after that the tree gets modified. |
Ah, I always forget that the post-canon trees aren't that helpful for diagnosing RA issues. Immediately prior to the register allocator, we should see
Do those |
Yes, I believe that part works as intended. I must admit that my hunt is based on partial understanding. Especially the wildcard stuff is black magic for me, and the book hasn't arrived. So it's more throwing sticks at the problem until it goes away. BTW, I replaced this in inset() in my local copy:
It looks like a genuine bug to me also present in the original LCC. And it's probably completely harmless. Right now I'm staring now at how spillr() operates. But I should go to sleep. |
|
Don't worry: the original LCC expression has the same equality operator. The reloads appear to be created in-place in the tree. That disproofs my reversal theory. |
Ahh, now this is starting to make more sense. Here's the output of
The nodes will execute in the order produced by a left-to-right postorder visit of the tree, so that tree would execute like so:
Because the result of (2) is still live in It is possible that we will need to change the lcc spiller to do something gt1-specific. I'm not sure that the architecture-independent spiller will actually work out. |
We may want to try adding a |
I goes wrong when it wants to inject a 3rd spill. The debug output is hard to read for me, even having stared at it for almost 20 hours now. Conceptually, there's no reason to use more than 2 temporary variables. And even if vAC is constantly allocated and freed, there shouldn't arise a lock if all goes in the right order. But somehow it does... |
This is the sequence I think should be emitted. There is no need for special treatment when tracking the occupancy of vAC. From what I can see, the compiler is pretty close to emitting this. It just trips over its own feet at some point. Maybe it is releasing vAC just a bit later than really necessary. Something small.
|
From the logs, I think that the RA is creating this sequence when it spills r1:
(2b) forces a spill of vAC, so the sequence then changes to this:
This sequence contains an unresolvable register conflict: (2b) and (6b) both require |
You're onto something. The
into this:
But here we expect a LOAD from vAC to r1 in the first argument to MUL and it isn't there. (Your 2b line). However, during this transformation there is this line being logged:
From this I assumed all the time it was doing the right thing. But we don't see that LOAD node actually appear in the tree. This debug logging is so hard to follow... |
Here is
The call to |
The results of targeting inserted reloads were being silently discarded, which resulted in kervinck#76. These changes stop discarding these results and label/reduce any resulting copies.
A set of changes that fixes this specific instance is here: master...pgavlin:pgavlin/spill I am not terribly confident in these changes, however. I think that the result of |
I have some doubts as well. It feels not quite right to protrude Gigatron changes outside gt1.md all the way into lburg... |
The This small set of edits is the fix: master...pgavlin:pgavlin/spill#diff-26094ddff7f8a9016fd6d0f421cb26d2R830 The fix should be target-independent as well, but I'm not totally confident in that. |
For my understanding (my book hasn't arrived yet, and I dearly start to need it):
I tried to test, but when I clone your repo en checkout 'spill', 'make lcc' works but 'make ctest' crashes:
I will be offline for a couple of days for surgery. Please feel free to commit to the main repo if you think it's good. You're the best judge of that :-) |
The book was supposed to arrive today, but it didn't. I've been doing some testing instead, focusing on the effect of just the proposed reprune() change for now (that seems to be the meat of the fix):
Gives:
The above works, and is roughly what you expect. The |
Just for laughs, the following from #77 results in a spilling fest.
It generates no less than 3 temporary variables.
One temp is to store the constant -1. The others for getting the operands for subtraction in the right order. Ironically, the order doesn't matter, because we're only testing for equality. One mitigation is to replace SUB with BXOR here. (I already have that prepared in another workspace). |
I found this article by Fraser and Hanson on LCC's register spilling: This might be helpful to understand what is going on in gen.c |
This is a nice reference article on the code generation: And here a report on retargeting LCC to a DSP. It might be of interest as they had to overcome register constraints as well. LCC is introduced in chapter 4. |
Party time: my LCC book finally arrived |
Due to personal circumstances I'm unable to focus on register allocation. Too difficult. [ As a side project, we have 6502 emulation in a pretty advanced state now. This might open up a path to use cc65 as a C compiler. ] |
This issue should be closed since glcc fixes it. |
The text was updated successfully, but these errors were encountered: