Assertion "LowerFormalArguments didn't emit the correct number of values!" failed #57
Comments
shepmaster
added
A-llvm
|
This is occurring because For some reason, our I can verify that this is causing it because if I add more registers to the calling convention It compiles successfully. |
|
I think we're missing something like this (taken from for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
// The mips ABIs for returning structs by value requires that we copy
// the sret argument into $v0 for the return. Save the argument into
// a virtual register so that we can access it from the return points.
if (Ins[i].Flags.isSRet()) {
unsigned Reg = MipsFI->getSRetReturnReg();
if (!Reg) {
Reg = MF.getRegInfo().createVirtualRegister(
getRegClassFor(ABI.IsN64() ? MVT::i64 : MVT::i32));
MipsFI->setSRetReturnReg(Reg);
}
SDValue Copy = DAG.getCopyToReg(DAG.getEntryNode(), DL, Reg, InVals[i]);
Chain = DAG.getNode(ISD::TokenFactor, DL, MVT::Other, Copy, Chain);
break;
}
} |
|
I think this is not actually it, it's just something similar. |
|
It's strange that there's an extra argument for the return value, but |
|
It looks like our |
|
Have pushed to a WIP patch avr-rust/llvm/branches/issue-57 Fixing the bug triggers another bug in |
It's better to have release mode in the long run, and this works around avr-rust/rust-legacy-fork#57.
dylanmckay
changed the title
|
Here is patch which should fix this bug: https://gist.github.com/brainlag/51928996f539bfafcfe0531e5dfd6236 It is based on @dylanmckay WIP patch. He got it almost right. The reason why some other tests fail is because parameters which are i32 or bigger gets split into 2 or more parameters and you have to skip over these. |
|
Your patch works fine, and doesn't regress the testsuite either! Upstreamed in r325474. |
Should there be a new test added for the original bug? |
|
This is the test i used https://gist.github.com/brainlag/b6498d15947c2be343403d6d901f8349 |
The test is taken from avr-rust/rust-legacy-fork#57 The originally implementation of struct return lowering was made in r325474. Patch by Peter Nimmervoll llvm-svn=327967
The test is taken from avr-rust/rust-legacy-fork#57 The originally implementation of struct return lowering was made in r325474. Patch by Peter Nimmervoll git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@327967 91177308-0d34-0410-b5e6-96231b3b80d8
|
Wait, should this be closed? |
|
I upstreamed this patch to LLVM, but then I thought it broke the testsuite. I just doubled checked and it didn't, so I'll cherry-pick this now. |
This commit cherry-picks the fix for #57. This corresponds to the upstream LLVM commit r325474.
|
Cherry-picked in 567801d. |
…uired by the ABI
When returning structs, the fields need to be reversed. Without this,
the ABI of the generated executables is not compatible with avr-gcc,
nor will LLVM allow it to be compiled.
Here is the assertion message when complex structs are used:
Impossible reg-to-reg copy
UNREACHABLE executed at llvm/lib/Target/AVR/AVRInstrInfo.cpp!
This was first noticed in avr-rust/rust-legacy-fork#92.
Description by Peter
The AVR couldn't handle returning structs like this at all (see avr-rust/rust-legacy-fork#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.
Patch by Peter Nimmervoll (@brainlag) and Tim Neumann (@TimNN)
EDIT (@dylanmckay): Here is another reproduction caused with non-i128 types. This was originally raised on #89. We should double check whatever fix is made fixes both of these.
The text was updated successfully, but these errors were encountered: