Closed TimNN closed 5 years ago
To debug this issue, I added an extra assert in getPhysicalRegisterVT
, which is triggered by the repro instead of the one above: https://gist.github.com/2255d0d389b30edbaea43e26bbdbdd1c
While I don't really understand the code, the way it is written suggests to me that the for loop not finding a match should be an error (If someone could confirm that, that would be great).
I ran the LLVM testsuite with the extra assert applied, and except for 11k unsupported tests and 7 failing AVR tests (none of which is hitting the assertion, they are likely failing due to other patches), everything passes.
Alright, so I don't believe I can properly explain why I think the following, nor am I even remotely sure that it is correct, however here are some thoughts:
bb41
(the mul
and the actual call
).SU1
is scheduled first.SU13
is attempted to be scheduled later but there is interference with SU1
. The interfering register is 53
, which is one higher than last index of a physical register and represents a special "CallResource" register. The interference is registered.At this point, I think that I know what is going on, but once again I have no idea how to fix this.
Not quite progress, but:
First of all, here is a much further (by hand) minimized IR:
target triple = "avr-unknown-unknown"
define void @"main"() addrspace(1) {
start:
%0 = or i64 undef, undef
br i1 undef, label %mul_and_call, label %fail
mul_and_call:
%1 = mul i64 %0, %0
call addrspace(1) void @"three_ints"(i64 undef, i64 %1, i64 %0)
ret void
fail:
ret void
}
declare void @"three_ints"(i64, i64, i64) addrspace(1)
Then, two graphs:
O0
O1
Things to note:
SU1
is the the call to three_ints
and SU2
is the call to mul
.three_ints
requires some arguments to be passed on the stack.ADJCALLSTACK
instructions are (or can be) paired (i.e. we have on down/call/up
sequence before we have the next down/call/up
sequence).down/down/call/up/call/up
. I believe that this may be the issue.cc @brainlag: You previously commented on an issue with ADJCALLSTACK*
(#103), do you have any ideas here?
The "bad" transformation happens in RegReductionPQBase::initNodes
, more specifically in PrescheduleNodesWithMultipleUses
. This means that either
PrescheduleNodesWithMultipleUses
a) PrescheduleNodesWithMultipleUses
performs an unsafe transformation or
b) the AVR backend doesn't correctly handle the result.
It would be extremely helpful if someone could weigh in on the following questions (which determines if this is a general LLVM or an AVR-specific bug (cc @dylanmckay)):
Are nested ADJCALLSTACK-
DOWN
/ UP
pairs valid? i.e. is down/down/up/up
valid or only down/up/down/up
?
I would say that nested ADJCALLSTACK down/up pairs are not valid but only some real llvm developers on the mailing list can confirm that for you.
It is really hard to say if a bug is llvm bug or a AVR backend bug. Often the AVR code triggers a bug in llvm no other backend does, because AVR doesn't have enough registers.
Thanks for the reply! I guess I'll write to the mailing list again.
Well, isn't it nice if a reply to a question starts with "The answer is probably: Nobody knows" :weary:.
In any case, the reply contains some interesting information, the most interesting being
The reason that brought me looking into the whole issue was some pattern getting turned into a compiler library call late. Back then I sidestepped the whole discussion by stopping to emit ADJCALLSTACK UP/DOWN of zero byte call frames https://reviews.llvm.org/D42006 so they wouldn’t nest anymore for the helpers functions in question…
Adding a similar patch to AVR fixes my repro (haven't tried it on all of libcore yet), so maybe that is the way to go.
Patch upstreamed, was already and still is in the cherry-pick.
The following IR was obtained by compiling libcore with
-C opt-level=1
to IR and then minimizing. The LLVM version is from a recent rust-lang/rust commit with some patches to work around other issues.Compile with
llc -O1
to reproduce.Edit: smaller repro below.
Output: