avr-rust/rust-legacy-fork

Assertion "LowerFormalArguments didn't emit the correct number of values!" failed

Closed this issue · 14 comments

target triple = "avr-atmel-none"

; Function Attrs: norecurse nounwind readonly uwtable
define i128 @"_ZN43_$LT$i32$u20$as$u20$core..fmt..num..Int$GT$7to_u12817haa6ed143bf6636caE"(i32* noalias nocapture readonly dereferenceable(4)) unnamed_addr #6 {
start:
  %1 = load i32, i32* %0, align 4
  %2 = sext i32 %1 to i128
  ret i128 %2
}

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.

declare i8 @do_something(i8 %val)

define { i1, i8 } @main(i8) #2 {
entry:
  %1 = call zeroext i8 @do_something(i8 zeroext %0)
  %2 = insertvalue { i1, i8 } { i1 true, i8 undef }, i8 %1, 1
  ret { i1, i8 } %2
}
CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8143: void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&): Assertion `InVals.size() == Ins.size() && "LowerFormalArguments didn't emit the correct number of values!"' failed.
#0 0x0000000001721085 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x1721085)
#1 0x000000000171f09e llvm::sys::RunSignalHandlers() (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x171f09e)
#2 0x000000000171f202 SignalHandler(int) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x171f202)
#3 0x00007fa62aaf3330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
#4 0x00007fa629cafc37 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x36c37)
#5 0x00007fa629cb3028 abort (/lib/x86_64-linux-gnu/libc.so.6+0x3a028)
#6 0x00007fa629ca8bf6 (/lib/x86_64-linux-gnu/libc.so.6+0x2fbf6)
#7 0x00007fa629ca8ca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
#8 0x00000000015c0f83 llvm::SelectionDAGISel::LowerArguments(llvm::Function const&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x15c0f83)
#9 0x00000000016091cc llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x16091cc)
#10 0x000000000160ac3c llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) [clone .part.948] (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x160ac3c)
#11 0x00000000010e8523 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x10e8523)
#12 0x000000000138ba03 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x138ba03)
#13 0x000000000138baac llvm::FPPassManager::runOnModule(llvm::Module&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x138baac)
#14 0x000000000138c80f llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x138c80f)
#15 0x0000000000697f74 compileModule(char**, llvm::LLVMContext&) (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x697f74)
#16 0x0000000000646a90 main (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x646a90)
#17 0x00007fa629c9af45 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f45)
#18 0x000000000068d2b5 _start (/home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc+0x68d2b5)
Stack dump:
0.	Program arguments: /home/cactus/prog/rust/rust-avr/build/build/x86_64-unknown-linux-gnu/llvm/bin/llc -mcpu=atmega328p a.ll 
1.	Running pass 'Function Pass Manager' on module 'a.ll'.
2.	Running pass 'AVR DAG->DAG Instruction Selection' on function '@"_ZN43_$LT$i32$u20$as$u20$core..fmt..num..Int$GT$7to_u12817haa6ed143bf6636caE"'

This is occurring because SelectionDAGISel::LowerArguments notices that FuncInfo->CanLowerReturn is false and so it attempts to convert the return value into an argument.

For some reason, our LowerFormalArguments is not doing what it should be in this case.

I can verify that this is causing it because if I add more registers to the calling convention

  def RetCC_AVR : CallingConv
  <[
    // i8 is returned in R24.
    CCIfType<[i8], CCAssignToReg<[R24]>>,

    // i16 are returned in R25:R24, R23:R22, R21:R20 and R19:R18.
    CCIfType<[i16], CCAssignToReg<[R25R24, R23R22, R21R20, R19R18, R17R16, R15R14, R13R12, R11R10]>>
  ]>;

It compiles successfully.

I think we're missing something like this (taken from MipsISelLowering.cpp)

  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 analyzeArguments is only populating ArgLocs with one element.

It looks like our parseFunctionArgs method is directly reading the arguments from a Function object, and not the arguments supplied by the SelectionDAGBuilder - this will be why we are only lowering one argument but not two.

Have pushed to a WIP patch avr-rust/llvm/branches/issue-57

Fixing the bug triggers another bug in add.ll and a bunch of other tests.

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.

and doesn't regress the testsuite either!

Should there be a new test added for the original bug?

@brainlag's test added in r327967

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.

Cherry-picked in 567801d.