rust-legacy-fork
rust-legacy-fork copied to clipboard
avr-rust
UNREACHABLE Impossible reg-to-reg copy
This bug occurs in my LLVM 6.0 branch at #91 when I compile stock libcore.
Impossible reg-to-reg copy
UNREACHABLE executed at /home/dylan/projects/llvm-project/llvm/lib/Target/AVR/AVRInstrInfo.cpp:75!
#0 0x00007fd02d6f8cc6 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/dylan/projects/llvm-project/llvm/lib/Support/Unix/Signals.inc:398:11
#1 0x00007fd02d6f8ec9 PrintStackTraceSignalHandler(void*) /home/dylan/projects/llvm-project/llvm/lib/Support/Unix/Signals.inc:462:1
#2 0x00007fd02d6f7260 llvm::sys::RunSignalHandlers() /home/dylan/projects/llvm-project/llvm/lib/Support/Signals.cpp:0:5
#3 0x00007fd02d6f92ba SignalHandler(int) /home/dylan/projects/llvm-project/llvm/lib/Support/Unix/Signals.inc:0:3
#4 0x00007fd02c5d6dd0 __restore_rt (/usr/lib/libpthread.so.0+0x11dd0)
#5 0x00007fd02b958860 __GI_raise (/usr/lib/libc.so.6+0x34860)
#6 0x00007fd02b959ec9 __GI_abort (/usr/lib/libc.so.6+0x35ec9)
#7 0x00007fd02d5f71f0 llvm::install_out_of_memory_new_handler() /home/dylan/projects/llvm-project/llvm/lib/Support/ErrorHandling.cpp:193:0
#8 0x00007fd032637599 llvm::AVRInstrInfo::copyPhysReg(llvm::MachineBasicBlock&, llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>, llvm::DebugLoc const&, unsigned int, unsigned int, bool) const /home/dylan/projects/llvm-project/llvm/lib/Target/AVR/AVRInstrInfo.cpp:0:7
#9 0x00007fd03004dd3b (anonymous namespace)::ExpandPostRA::LowerCopy(llvm::MachineInstr*) /home/dylan/projects/llvm-project/llvm/lib/CodeGen/ExpandPostRAPseudos.cpp:166:7
#10 0x00007fd03004d189 (anonymous namespace)::ExpandPostRA::runOnMachineFunction(llvm::MachineFunction&) /home/dylan/projects/llvm-project/llvm/lib/CodeGen/ExpandPostRAPseudos.cpp:212:23
#11 0x00007fd030230e61 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/dylan/projects/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:62:8
#12 0x00007fd02f76fdff llvm::FPPassManager::runOnFunction(llvm::Function&) /home/dylan/projects/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1520:27
#13 0x00007fd02f770152 llvm::FPPassManager::runOnModule(llvm::Module&) /home/dylan/projects/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1541:16
#14 0x00007fd02f7709b0 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/dylan/projects/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1597:27
#15 0x00007fd02f770436 llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/dylan/projects/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1700:16
#16 0x00007fd02f770f21 llvm::legacy::PassManager::run(llvm::Module&) /home/dylan/projects/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1731:3
#17 0x00005579e90cf34a compileModule(char**, llvm::LLVMContext&) /home/dylan/projects/llvm-project/llvm/tools/llc/llc.cpp:577:41
#18 0x00005579e90cd800 main /home/dylan/projects/llvm-project/llvm/tools/llc/llc.cpp:347:13
#19 0x00007fd02b944f4a __libc_start_main (/usr/lib/libc.so.6+0x20f4a)
#20 0x00005579e90cceea _start (./bin/llc+0x22eea)
Stack dump:
0. Program arguments: ./bin/llc -march=avr -mcpu=atmega328p bugpoint-reduced-simplified.ll -o /dev/null
1. Running pass 'Function Pass Manager' on module 'bugpoint-reduced-simplified.ll'.
2. Running pass 'Post-RA pseudo instruction expansion pass' on function '@_ZN3lib3num7flt2dec8strategy5grisu22max_pow10_no_more_than17h83b3c65a945b5158E'
Note that this is coming from @_ZN3lib3num7flt2dec8strategy5grisu22max_pow10_no_more_than17h83b3c65a945b5158E - the Grisu float-string algorithm.
I have minimised down to a testcase, and also collected llc debug outputs
target triple = "avr-unknown-unknown"
target datalayout = "e-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
define { i8, i32 } @chapstick() {
start:
ret { i8, i32 } zeroinitializer
}
You can see the build artifacts here.
lib::num::flt2dec::strategy
In our own libcore, we've disabled a lot of this code because it's simply too big. This may not be an extra blocker from moving to LLVM 6.
Do I understand this code correct. It tries to return a struct with the fields initialized to zero?
And if this is the machine code:
bb.0.start:
%2:ld8 = LDIRdK 0
%3:dldregs = LDIWRdK 0
$r21r20 = COPY %2
$r23r22 = COPY %3
$r24 = COPY %3
RET implicit $r21r20, implicit $r23r22, implicit $r24
Then the for some reason the copy instructions for 16bit registers and 8bit registers get mixed up. This doesn't sound to hard to fix.
It tries to return a struct with the fields initialized to zero?
That seems like it to me!
This doesn't sound to hard to fix.
Awesome!
~~~The patch is pretty messy because my C++ is in a really rusty state and I'm unable to reverse the const vector of the source registers.~~~
~~~But here we go [gist](https://gist.github.com/brainlag/309d47cd78aedc081773d7e14a65468d):~~~
```diff
diff --git a/lib/Target/AVR/AVRISelLowering.cpp b/lib/Target/AVR/AVRISelLowering.cpp
index 7ac8a136e6b..4f7d3d6a815 100644
--- a/lib/Target/AVR/AVRISelLowering.cpp
+++ b/lib/Target/AVR/AVRISelLowering.cpp
@@ -1371,16 +1371,12 @@ AVRTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
MachineFunction &MF = DAG.getMachineFunction();
unsigned e = RVLocs.size();
- // Reverse splitted return values to get the "big endian" format required
- // to agree with the calling convention ABI.
- if (e > 1) {
- std::reverse(RVLocs.begin(), RVLocs.end());
- }
-
SDValue Flag;
SmallVector<SDValue, 4> RetOps(1, Chain);
// Copy the result values into the output registers.
- for (unsigned i = 0; i != e; ++i) {
+ // Reverse loop to get the "big endian" format required
+ // to agree with the calling convention ABI.
+ for (int i = (e - 1); i < 0; --i) {
CCValAssign &VA = RVLocs[i];
assert(VA.isRegLoc() && "Can only return in registers!");
diff --git a/test/CodeGen/AVR/rust-avr-bug-92.ll b/test/CodeGen/AVR/rust-avr-bug-92.ll
new file mode 100644
index 00000000000..f93bf7a152d
--- /dev/null
+++ b/test/CodeGen/AVR/rust-avr-bug-92.ll
@@ -0,0 +1,6 @@
+; RUN: llc < %s -march=avr | FileCheck %s
+
+define { i8, i32 } @chapstick() {
+start:
+ ret { i8, i32 } zeroinitializer
+}
\ No newline at end of file
```
Can you narrow down what exactly the problem is? Is it just that the condition (if (e > 1)) has been removed? Is it that the call to std::reverse is "broken"?
I wonder if it would still work if the only change was removing if (e > 1) {.
Damn, I should have run the tests before. It breaks a lot of stuff. It made so much sense.
I think I know whats going on. The AVR couldn't handle returning structs like this at all (see #57). It only expects to ever return an iXX and only when XX is >=32 it needs to reverse the registers because of the calling convention. Now for this bug where we have { i8, i32 } and registers {[0] = i8, [1] = i16, [2] = i16} it only should reverse registers 1 and 2 but not 0.
https://gist.github.com/brainlag/4d9217eefcac1f06df682415975d97c3
diff --git a/lib/Target/AVR/AVRISelLowering.cpp b/lib/Target/AVR/AVRISelLowering.cpp
index 7ac8a136e6b..e0bb2f317d1 100644
--- a/lib/Target/AVR/AVRISelLowering.cpp
+++ b/lib/Target/AVR/AVRISelLowering.cpp
@@ -1346,7 +1346,7 @@ AVRTargetLowering::CanLowerReturn(CallingConv::ID CallConv,
CCState CCInfo(CallConv, isVarArg, MF, RVLocs, Context);
auto CCFunction = CCAssignFnForReturn(CallConv);
- return CCInfo.CheckReturn(Outs, CCFunction);
+ return !MF.getFunction().getReturnType()->isStructTy() && CCInfo.CheckReturn(Outs, CCFunction);
}
SDValue
diff --git a/test/CodeGen/AVR/rust-avr-bug-92.ll b/test/CodeGen/AVR/rust-avr-bug-92.ll
new file mode 100644
index 00000000000..e76ce630c67
--- /dev/null
+++ b/test/CodeGen/AVR/rust-avr-bug-92.ll
@@ -0,0 +1,7 @@
+; RUN: llc < %s -march=avr | FileCheck %s
+
+; CHECK-LABEL: main
+define { i8, i32 } @main() {
+start:
+ ret { i8, i32 } zeroinitializer
+}
\ No newline at end of file
I think I know whats going on. The AVR couldn't handle returning structs like this at all (see #57). It only expects to ever return an iXX and only when XX is >=32 it needs to reverse the registers because of the calling convention. Now for this bug where we have { i8, i32 } and registers {[0] = i8, [1] = i16, [2] = i16} it only should reverse registers 1 and 2 but not 0.
This makes a lot of sense. Sounds like we need to adjust this logic and write some more calling convention tests.
I don't know if this produces correct output but it prevents the crash. This patch just avoids to return a struct through registers. Which only works for a few/small structs anyway.
I suspect that this patch will break the calling convention for all functions which return structs, as they will now always be passed on the stack. FWIW, code compiled with this patch will work with code compiled under the same patch, but it would not work when compiling against objects from an older version of LLVM or AVR-GCC generated objects.
I am fairly busy the next handful of days, but after that this bug is at the top of my priority list. If anybody wants to look at this in the interim (please do!), feel free to ask questions on this issue, or ping me on Gitter.
In the past to fix calling convention issues like this, I've taken a C file and compared it side-by-side with LLVM/GCC compiled code.
I've found that this is the best way to make sure our calling convention implementation is accurate. It looks like we are missing struct-return tests, which would be good to add.
I think I know whats going on. The AVR couldn't handle returning structs like this at all (see #57). It only expects to ever return an iXX and only when XX is >=32 it needs to reverse the registers because of the calling convention. Now for this bug where we have { i8, i32 } and registers {[0] = i8, [1] = i16, [2] = i16} it only should reverse registers 1 and 2 but not 0.
This makes a lot of sense. Sounds like we need to adjust this logic and write some more calling convention tests.
I have code for this, but it felt like a ugly hack to me and after testing with a couple of different structs I found it really hard to find structs which don't use the stack. If a struct doesn't fit within 3 (or maybe for 4?) registered it used the stack even without this patch.
I update with examples later.
I tried it again. If the struct is <= 64 bit you can pass it without the stack.
Examples
; RUN: llc < %s -march=avr | FileCheck %s
; CHECK-LABEL: retstruct1
define { i8, i32 } @retstruct1() {
start:
ret { i8, i32 } zeroinitializer
}
; CHECK-LABEL: retstruct2
define { i16, i32 } @retstruct2() {
start:
ret { i16, i32 } zeroinitializer
}
; CHECK-LABEL: retstruct3
define { i32, i32 } @retstruct3() {
start:
ret { i32, i32 } zeroinitializer
}
; CHECK-LABEL: retstruct4
define { i8, i64 } @retstruct4() {
start:
ret { i8, i64 } zeroinitializer
}
; CHECK-LABEL: retstruct5
define { i8, i32, i16 } @retstruct5() {
start:
ret { i8, i32, i16 } zeroinitializer
}
; CHECK-LABEL: retstruct6
define { i8, i32, i32 } @retstruct6() {
start:
ret { i8, i32, i32 } zeroinitializer
}
The resulting asm code
.text
.file "rust-avr-bug-92.1.ll"
.globl retstruct1 ; -- Begin function retstruct1
.p2align 1
.type retstruct1,@function
retstruct1: ; @retstruct1
; %bb.0: ; %start
ldi r22, 0
ldi r23, 0
ldi r24, 0
mov r20, r22
mov r21, r23
ret
.Lfunc_end0:
.size retstruct1, .Lfunc_end0-retstruct1
; -- End function
.globl retstruct2 ; -- Begin function retstruct2
.p2align 1
.type retstruct2,@function
retstruct2: ; @retstruct2
; %bb.0: ; %start
ldi r24, 0
ldi r25, 0
mov r22, r24
mov r23, r25
mov r20, r24
mov r21, r25
ret
.Lfunc_end1:
.size retstruct2, .Lfunc_end1-retstruct2
; -- End function
.globl retstruct3 ; -- Begin function retstruct3
.p2align 1
.type retstruct3,@function
retstruct3: ; @retstruct3
; %bb.0: ; %start
ldi r24, 0
ldi r25, 0
mov r22, r24
mov r23, r25
mov r20, r24
mov r21, r25
mov r18, r24
mov r19, r25
ret
.Lfunc_end2:
.size retstruct3, .Lfunc_end2-retstruct3
; -- End function
.globl retstruct4 ; -- Begin function retstruct4
.p2align 1
.type retstruct4,@function
retstruct4: ; @retstruct4
; %bb.0: ; %start
mov r30, r24
mov r31, r25
ldi r24, 0
ldi r25, 0
std Z+7, r24
std Z+8, r25
std Z+5, r24
std Z+6, r25
std Z+3, r24
std Z+4, r25
ldi r18, 0
st Z+, r18
st Z, r24
std Z+1, r25
ret
.Lfunc_end3:
.size retstruct4, .Lfunc_end3-retstruct4
; -- End function
.globl retstruct5 ; -- Begin function retstruct5
.p2align 1
.type retstruct5,@function
retstruct5: ; @retstruct5
; %bb.0: ; %start
ldi r22, 0
ldi r23, 0
ldi r24, 0
mov r20, r22
mov r21, r23
mov r18, r22
mov r19, r23
ret
.Lfunc_end4:
.size retstruct5, .Lfunc_end4-retstruct5
; -- End function
.globl retstruct6 ; -- Begin function retstruct6
.p2align 1
.type retstruct6,@function
retstruct6: ; @retstruct6
; %bb.0: ; %start
mov r30, r24
mov r31, r25
ldi r24, 0
ldi r25, 0
std Z+7, r24
std Z+8, r25
std Z+5, r24
std Z+6, r25
std Z+3, r24
std Z+4, r25
ldi r18, 0
st Z+, r18
st Z, r24
std Z+1, r25
ret
.Lfunc_end5:
.size retstruct6, .Lfunc_end5-retstruct6
; -- End function
; Declaring this symbol tells the CRT that it should
;copy all variables from program memory to RAM on startup
.globl __do_copy_data
; Declaring this symbol tells the CRT that it should
;clear the zeroed data section on startup
.globl __do_clear_bss
The patch which fixes the register ordering
diff --git a/lib/Target/AVR/AVRISelLowering.cpp b/lib/Target/AVR/AVRISelLowering.cpp
index e0bb2f317d1..4dc7efaa51c 100644
--- a/lib/Target/AVR/AVRISelLowering.cpp
+++ b/lib/Target/AVR/AVRISelLowering.cpp
@@ -1374,7 +1374,24 @@ AVRTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
// Reverse splitted return values to get the "big endian" format required
// to agree with the calling convention ABI.
if (e > 1) {
- std::reverse(RVLocs.begin(), RVLocs.end());
+ // some hackery because SelectionDAGBuilder does not split up arguments properly
+ Type *retType = MF.getFunction().getReturnType();
+ if (retType->isStructTy()) {
+ if (retType->getNumContainedTypes() > 1 && retType->getNumContainedTypes() > e) {
+ for (unsigned i = 0, pos = 0; i < retType->getNumContainedTypes(); ++i) {
+ Type *field = retType->getContainedType(i);
+ if(field->isIntegerTy() && field->getIntegerBitWidth() > 16) {
+ int Size = field->getIntegerBitWidth() / 16;
+ std::reverse(RVLocs.begin()+ pos, RVLocs.end() + pos + Size);
+ pos += Size;
+ } else {
+ pos++;
+ }
+ }
+ }
+ } else {
+ std::reverse(RVLocs.begin(), RVLocs.end());
+ }
}
SDValue Flag;
Since @dylanmckay has been busy, and I've been trying to get engaged in this projct, I applied the patch and tried to compile libcore.
After working around #95 by lowering the optimization level, I got a new crash:
rustc: /home/admin/Software/avr-rust/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8736: void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&): Assertion `InVals.size() == Ins.size() && "LowerFormalArguments didn't emit the correct number of values!"' failed.
(gdb) bt
#0 0x00007ffff71f8860 in raise () from /usr/lib/libc.so.6
#1 0x00007ffff71f9ec9 in abort () from /usr/lib/libc.so.6
#2 0x00007ffff71f10bc in __assert_fail_base () from /usr/lib/libc.so.6
#3 0x00007ffff71f1133 in __assert_fail () from /usr/lib/libc.so.6
#4 0x00007fffeca26c14 in llvm::SelectionDAGISel::LowerArguments (
this=this@entry=0x7fffb61d9000, F=...)
at /home/admin/Software/avr-rust/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8735
#5 0x00007fffeca77d6e in llvm::SelectionDAGISel::SelectAllBasicBlocks (
this=this@entry=0x7fffb61d9000, Fn=...)
at /home/admin/Software/avr-rust/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1402
#6 0x00007fffeca78d72 in llvm::SelectionDAGISel::runOnMachineFunction (
this=<optimized out>, mf=...)
at /home/admin/Software/avr-rust/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:466
#7 0x00007fffecce12c5 in llvm::MachineFunctionPass::runOnFunction (
this=0x7fffb61d9000, F=...)
at /home/admin/Software/avr-rust/src/llvm/lib/CodeGen/MachineFunctionPass.cpp:62
(gdb) l 8735
8730 DAG.getRoot(), F.getCallingConv(), F.isVarArg(), Ins, dl, DAG, InVals);
8731
8732 // Verify that the target's LowerFormalArguments behaved as expected.
8733 assert(NewRoot.getNode() && NewRoot.getValueType() == MVT::Other &&
8734 "LowerFormalArguments didn't return a valid chain!");
8735 assert(InVals.size() == Ins.size() &&
8736 "LowerFormalArguments didn't emit the correct number of values!");
8737 DEBUG({
8738 for (unsigned i = 0, e = Ins.size(); i != e; ++i) {
8739 assert(InVals[i].getNode() &&
I poked around a bit, but as ignorant as I am about LLVM, it wasn't very insightful.
EDIT: I guess one thing is worth adding. If I counted fields correctly, the length of InVals is 8, while the length of Ins is 16.
Welcome to the club! :smile_cat:
Thanks! :smile:
In that case, does @brainlag have any more ideas?
I just checked, @brainlag, and the patch for #57 is the very last commit I have. I am using @shepmaster's rebase here.
Ok, you should pin the problem down by making a reduced testcase. Then create a new bug with the LLVM IR code that triggers the bug.
Whoops! It seems that I was using the wrong version of llc. Nevermind.
Thanks for the link about reduced testcases, though. I always wondered how those got generated. :smile:
@brainlag to double-check: should both this patch and this patch be applied, or does the second one replace the first?
Only the second one.
Cool. I've crossed out the first one to help in the future.
The current patch causes a few test failures.
I fixed the ones in add.ll and sub.ll in this patch. Two other tests are failing.
The tests are failing because registers are changing. I am unsure if the function calling convention argument registers have changed. The two tests I fixed up made the IR significantly uglier (see add+sub).
IIRC this patch did not break any tests. There must be something else going on.
I suspect these two tests are existing failures - I'm going to check again.
Is there an update on this? If this hasn't been fixed, is this a good problem for a beginner (partially to Rust and completely to LLVM)?
Yes, this should be good, @dylanmckay just need to verify it and upstream it.
Hi!
I recently tried to build avr-rust at the current Rust HEAD and attempted to build stock libcore. (I'm not really sure if that is supposed to work or not). After patching rustc to handle separate program-data address spaces (I'll send a PR for that ~soon), I ran into the impossible reg-to-reg copy issue. I applied the "current patch" referenced above, however that still fails with:
llc: /home/logic/avr/src/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8603: std::pair<llvm::SDValue, llvm::SDValue> llvm::TargetLowering::LowerCallTo(llvm::TargetLowering::CallLoweringInfo&) const: Assertion `EVT(CLI.Ins[i].VT) == InVals[i].getValueType() && "LowerCall emitted a value with the wrong type!"' failed.
Stack dump:
0. Program arguments: llc min0.ll
1. Running pass 'Function Pass Manager' on module 'min0.ll'.
2. Running pass 'AVR DAG->DAG Instruction Selection' on function '@core_iter_TakeWhile_try_fold_closure'
[...]
#9 0x00007f58ae86dcb9 llvm::TargetLowering::LowerCallTo(llvm::TargetLowering::CallLoweringInfo&) const /home/logic/avr/src/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8602:0
#10 0x00007f58ae860d25 llvm::SelectionDAGBuilder::lowerInvokable(llvm::TargetLowering::CallLoweringInfo&, llvm::BasicBlock const*) /home/logic/avr/src/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6395:0
#11 0x00007f58ae861617 llvm::SelectionDAGBuilder::LowerCallTo(llvm::ImmutableCallSite, llvm::SDValue, bool, llvm::BasicBlock const*) /home/logic/avr/src/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6508:0
#12 0x00007f58ae86415c llvm::SelectionDAGBuilder::visitCall(llvm::CallInst const&) /home/logic/avr/src/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7058:0
[...]
Attaching the debugger showed the following:
The assertion happens while lowering %3 = call addrspace(1) { i8, i16 } @core_iter_LoopState_from_try(i16 %2) (from calling I.dump() in frame #12 above). The issue appears to be that while CLI.Ins contains the types {i8, i16}, InVals contains the types {i16, i8} (note the swapped order).
To me, that seems very connected to the proposed patch in this PR, so I'm reporting this here rather than in a new issue.
Looking at AVRISelLowering.cpp, there appear to be a total of three places where arguments are reversed:
- The
LowerReturnfunction handled by the patch. - The
LowerCallResultfunction which looks very similar to (1) but isn't patched. - The
analyzeStandardArgumentswhich I have no idea about.