[LLVM 8.0] Assertion failed: Trying to add an operand to a machine instr that is already done!

[TimNN]

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.

; ModuleID = 'bugpoint-reduced-simplified.bc'
source_filename = "core.3a1fbbbh-cgu.0"
target datalayout = "e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
target triple = "avr-unknown-unknown"

; Function Attrs: cold noinline noreturn uwtable
define void @"core::str::slice_error_fail"(i16) unnamed_addr addrspace(0) #0 personality i32 (...) addrspace(1)* @rust_eh_personality {
start:
  %char_range = alloca { i16, i16 }, align 1
  br i1 undef, label %"<core::option::Option<T>>::unwrap.exit.thread", label %bb11.i.i

"<core::option::Option<T>>::unwrap.exit.thread": ; preds = %start
  br label %"core::char::methods::<impl char>::len_utf8.exit"

bb11.i.i:                                         ; preds = %start
  %1 = bitcast { i16, i16 }* %char_range to i8*
  %2 = icmp ult i32 undef, 65536
  %..i = select i1 %2, i16 3, i16 4
  br label %"core::char::methods::<impl char>::len_utf8.exit"

"core::char::methods::<impl char>::len_utf8.exit": ; preds = %bb11.i.i, %"<core::option::Option<T>>::unwrap.exit.thread"
  %3 = phi i8* [ %1, %bb11.i.i ], [ undef, %"<core::option::Option<T>>::unwrap.exit.thread" ]
  %_0.0.i12 = phi i16 [ %..i, %bb11.i.i ], [ 1, %"<core::option::Option<T>>::unwrap.exit.thread" ]
  %4 = add i16 %_0.0.i12, %0
  store i16 %4, i16* undef, align 1
  store i8* %3, i8** undef, align 1
  unreachable
}

declare i32 @rust_eh_personality(...) unnamed_addr addrspace(1) #1

attributes #0 = { cold noinline noreturn uwtable }
attributes #1 = { "target-cpu"="atmega32u4" }

Output:

Assertion failed: ((isImpReg || Op.isRegMask() || MCID->isVariadic() || OpNo < MCID->getNumOperands() || isMetaDataOp) && "Trying to add an operand to a machine instr that is already done!"), function addOperand, file /Users/logic/Projects/rustc/dev/src/llvm/lib/CodeGen/MachineInstr.cpp, line 233.
Stack dump:
0.	Program arguments: llc -O1 min03-dem.ll
1.	Running pass 'Function Pass Manager' on module 'min03-dem.ll'.
2.	Running pass 'Tail Duplication' on function '@"core::str::slice_error_fail"'
0  llc                      0x000000010c0a2d7c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  llc                      0x000000010c0a3349 PrintStackTraceSignalHandler(void*) + 25
2  llc                      0x000000010c09fa1d llvm::sys::RunSignalHandlers() + 989
3  llc                      0x000000010c0a3fd5 SignalHandler(int) + 485
4  libsystem_platform.dylib 0x00007fff7b932f5a _sigtramp + 26
5  llc                      0x000000010be69fb0 llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::MachineInstr, true, true, void>, false, false>::operator*() const + 96
6  libsystem_c.dylib        0x00007fff7b75d312 abort + 127
7  libsystem_c.dylib        0x00007fff7b725368 basename_r + 0
8  llc                      0x000000010ad3b98f llvm::MachineInstr::addOperand(llvm::MachineFunction&, llvm::MachineOperand const&) + 831
9  llc                      0x000000010ad3c0b0 llvm::MachineInstr::MachineInstr(llvm::MachineFunction&, llvm::MachineInstr const&) + 400
10 llc                      0x000000010ad3c105 llvm::MachineInstr::MachineInstr(llvm::MachineFunction&, llvm::MachineInstr const&) + 37
11 llc                      0x000000010ad18225 llvm::MachineFunction::CloneMachineInstr(llvm::MachineInstr const*) + 69
12 llc                      0x000000010ad1827d llvm::MachineFunction::CloneMachineInstrBundle(llvm::MachineBasicBlock&, llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>, llvm::MachineInstr const&) + 77
13 llc                      0x000000010b0a7503 llvm::TargetInstrInfo::duplicate(llvm::MachineBasicBlock&, llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>, llvm::MachineInstr const&) const + 163
14 llc                      0x000000010b09f225 llvm::TailDuplicator::duplicateInstruction(llvm::MachineInstr*, llvm::MachineBasicBlock*, llvm::MachineBasicBlock*, llvm::DenseMap<unsigned int, llvm::TargetInstrInfo::RegSubRegPair, llvm::DenseMapInfo<unsigned int>, llvm::detail::DenseMapPair<unsigned int, llvm::TargetInstrInfo::RegSubRegPair> >&, llvm::DenseSet<unsigned int, llvm::DenseMapInfo<unsigned int> > const&) + 421
15 llc                      0x000000010b09b489 llvm::TailDuplicator::tailDuplicate(bool, llvm::MachineBasicBlock*, llvm::MachineBasicBlock*, llvm::SmallVectorImpl<llvm::MachineBasicBlock*>&, llvm::SmallVectorImpl<llvm::MachineInstr*>&) + 1305
16 llc                      0x000000010b09a729 llvm::TailDuplicator::tailDuplicateAndUpdate(bool, llvm::MachineBasicBlock*, llvm::MachineBasicBlock*, llvm::SmallVectorImpl<llvm::MachineBasicBlock*>*, llvm::function_ref<void (llvm::MachineBasicBlock*)>*) + 265
17 llc                      0x000000010b09cf0b llvm::TailDuplicator::tailDuplicateBlocks() + 411
18 llc                      0x000000010b099bb9 (anonymous namespace)::TailDuplicateBase::runOnMachineFunction(llvm::MachineFunction&) + 169
19 llc                      0x000000010ad38721 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 449
20 llc                      0x000000010b3ea46b llvm::FPPassManager::runOnFunction(llvm::Function&) + 475
21 llc                      0x000000010b3eaa75 llvm::FPPassManager::runOnModule(llvm::Module&) + 117
22 llc                      0x000000010b3eb5bb (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 1515
23 llc                      0x000000010b3ead4e llvm::legacy::PassManagerImpl::run(llvm::Module&) + 366
24 llc                      0x000000010b3ec041 llvm::legacy::PassManager::run(llvm::Module&) + 33
25 llc                      0x0000000109fdce48 compileModule(char**, llvm::LLVMContext&) + 24104
26 llc                      0x0000000109fd6414 main + 5460
27 libdyld.dylib            0x00007fff7b6b1115 start + 1
28 libdyld.dylib            0x0000000000000003 start + 2224353007

[TimNN]

The issue is here:

https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AVR/AVRRegisterInfo.cpp#L152-L154

  if (MI.getOpcode() == AVR::FRMIDX) {
    MI.setDesc(TII.get(AVR::MOVWRdRr));
    MI.getOperand(FIOperandNum).ChangeToRegister(AVR::R29R28, false);

The FRMIDX has two inputs, whereas the MOVWRdRr instruction has only one. Thus just changing the operand produces an invalid instruction, which later causes the assertion if this particular code path is hit. Not sure how to write the fix for this, but maybe @dylanmckay can help here.

[TimNN]

Adding an MI.RemoveOperand(2); immediately after these lines fixes the issue (and should be safe to do, IIUC, since we know exactly which instruction we are modifying and that instruction has exactly three explicit parameters.

With this patch (and all the others I commented on) applied, I can now compile stock libcore on all optimization levels except -C opt-level 1, where I'm hitting LLVM ERROR: ran out of registers.

[shepmaster]

on all optimization levels except -C opt-level 1

So 0, 2, 3, s, and z all work? That's exciting!

[TimNN]

So 0, 2, 3, s, and z all work? That's exciting!

That's exactly what it means! 😄 (at least if "work" means "compiles", I wouldn't bet on it actually being compiled bug-free yet).

[shepmaster]

Is opt-level 1 the default of Cargo for anything? 3 is --release, and probably most AVR stuff should be s or z. I'm wondering if (with all the patches) this would be good enough to land as our master branch.

[TimNN]

Is opt-level 1 the default of Cargo for anything?

I don't think so.

I'm wondering if (with all the patches) this would be good enough to land as our master branch.

I think so. Although if we also upgrade to the latest rustc we'll need rust-lang#54993 or rust-lang#51576 as well.

(Also, I'm gonna see if I can figure something out about the "ran out of registers" thing as well).

[shepmaster]

if we also upgrade to the latest rustc we'll need

I don't know about need — that's not something that we currently support in AVR land anyway, right?

[shepmaster]

That is, that's why #84 is open, right?

[TimNN]

that's not something that we currently support in AVR land anyway, right

Actually, if we upgrade to LLVM 8.0 (which is what the current rustc master uses), we do need one of the patches (otherwise compilation will fail pretty early).

[TimNN]

Note that we don't need all of rust-lang#51576. However, since a recent LLVM commit the address space of program data (and thus functions and function pointers) is defined to be 1, so we need to specify that in the IR generated by rustc.

[shepmaster]

thus functions and function pointers

If I'm reading correctly, neither of those PRs would allow for a user to annotate "things" to specifically be in a given address space?

[TimNN]

If I'm reading correctly, neither of those PRs would allow for a user to annotate "things" to specifically be in a given address space?

At least my PR does not. I don't know about the other one, but I don't think so.

[TimNN]

I'm wondering if (with all the patches) this would be good enough to land as our master branch.

@shepmaster FYI: I've opened #115 with my current set of changes (although I'm still verifying that I didn't mess anything up while rebasing things).

[dylanmckay]

Upstreamed #112 in r346117.

Also raised LLVM bug PR39554 to track writing of proper unit tests for FRMIDX expansion.

[dylanmckay]

Thanks for the patch @TimNN!