[mips][microMIPS] Fix for "Cannot copy registers" assertion

[hvarga]

Summary

Patch is fixing assertion raised in case of copying 64-bit register into 32-bit one and vice versa. This assertion is raised in case when -mcpu=mips64r6 and -mattr=micromips are set for llc.

There are two test cases that are triggering this assertion. Test test/CodeGen/Mips/micromips-addiu.ll is for a case of copying 64-bit into 32-bit register and test test/CodeGen/Mips/micromips-gp-rc.ll is for a case of copying 32-bit into 64-bit register.

[sdardis]

I haven't been able to hit the assertion on test/CodeGen/Mips/micromips-addiu.ll with "-march=mips64r6 -mattr=micromips" with r260891. Would you be able to provide another test case?

[hvarga]

In D17068#353457, @sdardis wrote:

I haven't been able to hit the assertion on test/CodeGen/Mips/micromips-addiu.ll with "-march=mips64r6 -mattr=micromips" with r260891. Would you be able to provide another test case?

This is strange. My colleagues can also reproduce this assertion. What about the other one in this patch; did you reproduced it successfully? There are actually quite a number of test cases that are triggering the same assertion. But they are all failing because of two reasons; copying from 64-bit register into 32-bit one or copying from 32-bit register into 64-bit register. This is why I only added two test cases.

Anyway, I will list a few of those tests so you can try to reproduce it (files listed below are all located in the directory test/CodeGen/Mips):

• micromips-andi.ll
• micromips-li.ll
• micromips-sw-lw-16.ll
• micromips-shift.ll
• micromips-sw-lw-16.ll

[sdardis]

That is strange. The other test case demonstrates the stated issue.

As you've pointed out micromips-andi.ll demonstrates the problem and generates 32 to 64 bit registers and 64 to 32 bit register copies. Can you add a runline to that file for micromips64r6 and a comment like '; For micromips64, also check 32 to 64 bit registers and 64 to 32 bit register copies'.

LGTM with that change.

[sdardis]

Request: can you hold off committing this, vkalintiris has http://reviews.llvm.org/D16220 which changes a lot of test cases. I'll add it as a dependency.

[hvarga]

Rebased and modified according to the comments received from Simon Dardis.

[hvarga]

I will wait until D16220 is committed.

[vkalintiris]

Hi Hrvoje, you can go on and commit this and any relevant patches you might have that I was blocking with D16220.

[hvarga]

I am experiencing an issue with one of the test cases when I rebase master.
I have pinpointed that patch D15420 triggers following error in micromips-addiu.ll:

Unexpected call instruction for microMIPS.
UNREACHABLE executed at /home/rtrk/Projects/LLVM/llvm-osijek/lib/Target/Mips/MipsDelaySlotFiller.cpp:563!
#0 0x00000000018d19a7 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Support/Unix/Signals.inc:322:0
#1 0x00000000018d1cbc PrintStackTraceSignalHandler(void*) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Support/Unix/Signals.inc:380:0
#2 0x00000000018d03d4 llvm::sys::RunSignalHandlers() /home/rtrk/Projects/LLVM/llvm-osijek/lib/Support/Signals.cpp:44:0
#3 0x00000000018d14ce SignalHandler(int) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Support/Unix/Signals.inc:210:0
#4 0x00002b307b2f9340 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10340)
#5 0x00002b307bf76cc9 gsignal /build/eglibc-3GlaMS/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
#6 0x00002b307bf7a0d8 abort /build/eglibc-3GlaMS/eglibc-2.19/stdlib/abort.c:91:0
#7 0x0000000001878c23 bindingsErrorHandler(void*, std::string const&, bool) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Support/ErrorHandling.cpp:126:0
#8 0x0000000000dc3db4 getEquivalentCallShort(int) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Target/Mips/MipsDelaySlotFiller.cpp:565:0
#9 0x0000000000dc3fc9 (anonymous namespace)::Filler::runOnMachineBasicBlock(llvm::MachineBasicBlock&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Target/Mips/MipsDelaySlotFiller.cpp:604:0
#10 0x0000000000dc2726 (anonymous namespace)::Filler::runOnMachineFunction(llvm::MachineFunction&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/Target/Mips/MipsDelaySlotFiller.cpp:181:0
#11 0x000000000119b0b9 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/CodeGen/MachineFunctionPass.cpp:44:0
#12 0x00000000014b2fe6 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/IR/LegacyPassManager.cpp:1550:0
#13 0x00000000014b3179 llvm::FPPassManager::runOnModule(llvm::Module&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/IR/LegacyPassManager.cpp:1571:0
#14 0x00000000014b3514 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/IR/LegacyPassManager.cpp:1627:0
#15 0x00000000014b3c64 llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/IR/LegacyPassManager.cpp:1730:0
#16 0x00000000014b3ea5 llvm::legacy::PassManager::run(llvm::Module&) /home/rtrk/Projects/LLVM/llvm-osijek/lib/IR/LegacyPassManager.cpp:1762:0
#17 0x0000000000af6ec3 compileModule(char**, llvm::LLVMContext&) /home/rtrk/Projects/LLVM/llvm-osijek/tools/llc/llc.cpp:408:0
#18 0x0000000000af5c98 main /home/rtrk/Projects/LLVM/llvm-osijek/tools/llc/llc.cpp:211:0
#19 0x00002b307bf61ec5 __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:321:0
#20 0x0000000000af4539 _start (/home/rtrk/Projects/LLVM/llvm-osijek-build/bin/llc+0xaf4539)
Stack dump:
0.  Program arguments: /home/rtrk/Projects/LLVM/llvm-osijek-build/./bin/llc -march=mips -mcpu=mips64r6 -mattr=+micromips -relocation-model=pic -O3 
1.  Running pass 'Function Pass Manager' on module '<stdin>'.
2.  Running pass 'Mips Delay Slot Filler' on function '@main'

This error only happens in the case of -mcpu=mips64r6 -mattr=+micromips and only in micromips-addiu.ll. We are probably missing something for microMIPS64r6. So before I commit I need to see what is the problem here.

[hvarga]

I have set a dependency for D18181 because there is no need to fill the delay slot in case of microMIPSr6. Filling the delay slot, in this case, triggers the assertion as described in my D17068#370440 comment.

So with the patch D18181 applied, this assertion is not triggered anymore.

[hvarga]

It is discovered that test-suite in case of microMIPS is failing after committing this patch into mainline. test.log contained error messages like:

/tmp/ConditionalExpr-b127f7.s: Assembler messages:
/tmp/ConditionalExpr-b127f7.s:487: Error: invalid operands `lw16 $4,0($1)'
/tmp/ConditionalExpr-b127f7.s:489: Error: invalid operands `sw16 $4,0($1)'

I found out that i forgot to change MipsRegisterInfo::getPointerRegClass() in order to support GPRMM16 and GPRMM16_64 register classes. After this fix, manual execution of test-suite is passing. So please, review this change once again.

[hvarga]

Matthew reminded me about the bug we had in GCC for microMIPS that related to SW16. The bug was that we did not account for the special register class for SW16 where it allows $0 as a source instead of $16.

This patch introduced the same invalid behavior. After applying this patch (the previous version) CodeGen invalidly chooses register $1 as an operand to LW16 and SW16. But those instructions have 3-bit register fields. This means that LW16 and SW16 can only have a GPRs $2-$7, $16, $17.

But like I said, this is now fixed with the newest version of the patch.

[vkalintiris]

Overall the changes LGTM. However, could you reply to my comments before committing?

test/CodeGen/Mips/invalid_operands.ll

1–23

What is the goal of this test? Currently, you are testing for lw16 and the base register $2, but I don't see the motivation behind this.

test/CodeGen/Mips/micromips-addiu.ll

3

I don't think that we should add this comment to every test because it'll become confusing in the future as we add more RUN-lines. Could you pick a minimal test, or just one of the existing tests, and check this in a separate test-file?

4

-march= should be mips64 instead of mips. Similarly for the test cases below.

[hvarga]

test/CodeGen/Mips/invalid_operands.ll

1–23

The purpose of this test is to check whether the CodeGen selected lw16 with the base register equal to $2. Why? Because the previous version of this patch had a bug. If you try this test with the previous version of this patch, the CodeGen would select $1 as a base register.

Selecting $1 as a base register is an invalid behavior in case of LW16 instruction as only valid registers are $2-$7, $16, $17.

This was described in D17068#396756.

test/CodeGen/Mips/micromips-addiu.ll

3

I was thinking about that when I first implemented this patch. But I decided to change an existing test file. I could do that. I could copy those couple of test files and create the new test files.

4

Agreed. I will change this.

[vkalintiris]

test/CodeGen/Mips/invalid_operands.ll

1–23

Thanks for your explanation. I think that we should rename this file to something like lw16-base-reg.ll and add a very short description of what we are trying to test. Also, given that the base register can be different than $2, we should use a regex that matches all the possible values of that register.

test/CodeGen/Mips/micromips-addiu.ll

3

I could copy those couple of test files and create the new test files.

There's no need to copy the contents from several test files. We only have to pick the necessary tests that have 32-to-64 and 64-to-32 copies.

[hvarga]

test/CodeGen/Mips/invalid_operands.ll

1–23

Agreed.

test/CodeGen/Mips/micromips-addiu.ll

3

Well, I did that in the first place. I had only one test file that triggers 32-to-64 and 64-to-32 copies. But Simon, described in his comments D17068#354586 and D17068#353457, could not reproduce the bug that I and my colleagues experienced. That is why I had changed few more test files which were failing on my machine but are also failing on his own.

But yeah, I agree that we need only one test case that has a 32-to-64 and 64-to-32 copies and proves the fix implemented in this patch. So I will take a test in micromips-andi.ll and create a new test file based on that. This test fails both on my and Simon machine.

lib/Target/Mips/MicroMipsInstrInfo.td

def MicroMipsMemGPRMM16AsmOperand : AsmOperandClass {
   let Name = "MicroMipsMem";
   let RenderMethod = "addMicroMipsMemOperands";
   let ParserMethod = "parseMemOperand";
   let PredicateMethod = "isMemWithGRPMM16Base";
 }
 
 class mem_mm_4_generic : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPRMM16, simm4);
+  let MIOperandInfo = (ops ptr_rc, simm4);
   let OperandType = "OPERAND_MEMORY";
   let ParserMatchClass = MicroMipsMemGPRMM16AsmOperand;
 }
 
 def mem_mm_4 : mem_mm_4_generic {
   let EncoderMethod = "getMemEncodingMMImm4";
 }
 
def MicroMipsMemSPAsmOperand : AsmOperandClass {
   let Name = "MicroMipsMemSP";
   let RenderMethod = "addMemOperands";
   let ParserMethod = "parseMemOperand";
   let PredicateMethod = "isMemWithUimmWordAlignedOffsetSP<7>";
 }
 
 def mem_mm_sp_imm5_lsl2 : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPR32:$base, simm5:$offset);
+  let MIOperandInfo = (ops ptr_rc:$base, simm5:$offset);
   let OperandType = "OPERAND_MEMORY";
   let ParserMatchClass = MicroMipsMemSPAsmOperand;
   let EncoderMethod = "getMemEncodingMMSPImm5Lsl2";
 }
 
 def mem_mm_gp_imm7_lsl2 : Operand<i32> {
   let PrintMethod = "printMemOperand";
   let MIOperandInfo = (ops GPRMM16:$base, simm7_lsl2:$offset);
   let OperandType = "OPERAND_MEMORY";
   let EncoderMethod = "getMemEncodingMMGPImm7Lsl2";
 }
 
 def mem_mm_9 : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPR32, simm9);
+  let MIOperandInfo = (ops ptr_rc, simm9);
   let EncoderMethod = "getMemEncodingMMImm9";
   let ParserMatchClass = MipsMemSimm9AsmOperand;
   let OperandType = "OPERAND_MEMORY";
 }
 
 def mem_mm_12 : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPR32, simm12);
+  let MIOperandInfo = (ops ptr_rc, simm12);
   let EncoderMethod = "getMemEncodingMMImm12";
   let ParserMatchClass = MipsMemAsmOperand;
   let OperandType = "OPERAND_MEMORY";
 }
 
 def mem_mm_16 : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPR32, simm16);
+  let MIOperandInfo = (ops ptr_rc, simm16);
   let EncoderMethod = "getMemEncodingMMImm16";
   let ParserMatchClass = MipsMemAsmOperand;
   let OperandType = "OPERAND_MEMORY";
 }
 
 def MipsMemUimm4AsmOperand : AsmOperandClass {
   let Name = "MemOffsetUimm4";
   let SuperClasses = [MipsMemAsmOperand];
   let RenderMethod = "addMemOperands";
   let ParserMethod = "parseMemOperand";
   let PredicateMethod = "isMemWithUimmOffsetSP<6>";
 }
 
 def mem_mm_4sp : Operand<i32> {
   let PrintMethod = "printMemOperand";
-  let MIOperandInfo = (ops GPR32, uimm8);
+  let MIOperandInfo = (ops ptr_rc, uimm8);
   let EncoderMethod = "getMemEncodingMMImm4sp";
   let ParserMatchClass = MipsMemUimm4AsmOperand;
   let OperandType = "OPERAND_MEMORY";
 }
 
 def jmptarget_mm : Operand<OtherVT> {
   let EncoderMethod = "getJumpTargetOpValueMM";
 }
def MovePRegPairAsmOperand : AsmOperandClass {
   let PredicateMethod = "isMovePRegPair";
 }
 
 def movep_regpair : Operand<i32> {
   let EncoderMethod = "getMovePRegPairOpValue";
   let ParserMatchClass = MovePRegPairAsmOperand;
   let PrintMethod = "printRegisterList";
   let DecoderMethod = "DecodeMovePRegPair";
-  let MIOperandInfo = (ops GPR32Opnd, GPR32Opnd);
+  let MIOperandInfo = (ops ptr_rc, ptr_rc);
 }
 
 class MovePMM16<string opstr, RegisterOperand RO> :
 MicroMipsInst16<(outs movep_regpair:$dst_regs), (ins RO:$rs, RO:$rt),
                  !strconcat(opstr, "\t$dst_regs, $rs, $rt"), [],
                  NoItinerary, FrmR> {
   let isReMaterializable = 1;
 }
 
 /// A register pair used by load/store pair instructions.
 def RegPairAsmOperand : AsmOperandClass {
   let Name = "RegPair";
   let ParserMethod = "parseRegisterPair";
 }
 
 def regpair : Operand<i32> {
   let EncoderMethod = "getRegisterPairOpValue";
   let ParserMatchClass = RegPairAsmOperand;
   let PrintMethod = "printRegisterPair";
   let DecoderMethod = "DecodeRegPairOperand";
-  let MIOperandInfo = (ops GPR32Opnd, GPR32Opnd);
+  let MIOperandInfo = (ops ptr_rc, ptr_rc);
 }
 
 class StorePairMM<string opstr, InstrItinClass Itin = NoItinerary,
                   ComplexPattern Addr = addr> :
   InstSE<(outs), (ins regpair:$rt, mem_mm_12:$addr),
          !strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI, opstr> {
   let DecoderMethod = "DecodeMemMMImm12";
   let mayStore = 1;

lib/Target/Mips/MipsMachineFunction.cpp

unsigned MipsFunctionInfo::getGlobalBaseReg() {
 
   MipsSubtarget const &STI =
       static_cast<const MipsSubtarget &>(MF.getSubtarget());
 
   const TargetRegisterClass *RC =
       STI.inMips16Mode()
           ? &Mips::CPU16RegsRegClass
           : STI.inMicroMipsMode()
-                ? &Mips::GPRMM16RegClass
+                ? STI.hasMips64()
+                      ? &Mips::GPRMM16_64RegClass
+                      : &Mips::GPRMM16RegClass
                 : static_cast<const MipsTargetMachine &>(MF.getTarget())
                           .getABI()
                           .IsN64()
                       ? &Mips::GPR64RegClass
                       : &Mips::GPR32RegClass;
   return GlobalBaseReg = MF.getRegInfo().createVirtualRegister(RC);
 }
 

lib/Target/Mips/MipsRegisterInfo.cpp

 MipsRegisterInfo::MipsRegisterInfo() : MipsGenRegisterInfo(Mips::RA) {}
 
 unsigned MipsRegisterInfo::getPICCallReg() { return Mips::T9; }
 
 const TargetRegisterClass *
 MipsRegisterInfo::getPointerRegClass(const MachineFunction &MF,
                                      unsigned Kind) const {
   MipsABIInfo ABI = MF.getSubtarget<MipsSubtarget>().getABI();
-  return ABI.ArePtrs64bit() ? &Mips::GPR64RegClass : &Mips::GPR32RegClass;
+  bool inMicroMips = MF.getSubtarget<MipsSubtarget>().inMicroMipsMode();
+ 
+  return ABI.ArePtrs64bit() ?
+             inMicroMips ?
+                 &Mips::GPRMM16_64RegClass : &Mips::GPR64RegClass
+             : inMicroMips ?
+                 &Mips::GPRMM16RegClass : &Mips::GPR32RegClass;
 }
 
 unsigned
 MipsRegisterInfo::getRegPressureLimit(const TargetRegisterClass *RC,
                                       MachineFunction &MF) const {
   switch (RC->getID()) {
   default:
     return 0;

lib/Target/Mips/MipsRegisterInfo.td

// Reserved
   T0_64, T1_64, T2_64, T3_64, T4_64, T5_64, T6_64, T7_64,
   // Callee save
   S0_64, S1_64, S2_64, S3_64, S4_64, S5_64, S6_64, S7_64,
   // Not preserved across procedure calls
   T8_64, T9_64,
   // Reserved
   K0_64, K1_64, GP_64, SP_64, FP_64, RA_64)>;
 
+def GPRMM16_64 : RegisterClass<"Mips", [i64], 64, (add
+  // Callee save
+  S0_64, S1_64,
+  // Return Values and Arguments
+  V0_64, V1_64, A0_64, A1_64, A2_64, A3_64)>;
+
 def CPU16Regs : RegisterClass<"Mips", [i32], 32, (add
   // Return Values and Arguments
   V0, V1, A0, A1, A2, A3,
   // Callee save
   S0, S1)>;
 
 def CPU16RegsPlusSP : RegisterClass<"Mips", [i32], 32, (add
   // Return Values and Arguments

test/CodeGen/Mips/invalid_operands.ll

+; RUN: llc %s -march=mips -mcpu=mips32r3 -mattr=micromips -filetype=asm \
+; RUN: -relocation-model=pic -O3 -o - | FileCheck %s
+; RUN: llc %s -march=mips -mcpu=mips64r3 -mattr=micromips -filetype=asm \
+; RUN: -relocation-model=pic -O3 -o - | FileCheck %s
+
+%struct.T = type { i32 }
+
+$_ZN1TaSERKS_ = comdat any
+
+define linkonce_odr void @_ZN1TaSERKS_(%struct.T* %this, %struct.T* dereferenceable(4) %t) #0 comdat align 2 {
+entry:
+  %this.addr = alloca %struct.T*, align 4
+  %t.addr = alloca %struct.T*, align 4
+  %this1 = load %struct.T*, %struct.T** %this.addr, align 4
+  %0 = load %struct.T*, %struct.T** %t.addr, align 4
+  %V3 = getelementptr inbounds %struct.T, %struct.T* %0, i32 0, i32 0
+  %1 = load i32, i32* %V3, align 4
+  %V4 = getelementptr inbounds %struct.T, %struct.T* %this1, i32 0, i32 0
+  store i32 %1, i32* %V4, align 4
+  ret void
+}
+
+; CHECK: lw16 ${{[0-9]+}}, 0($2)

test/CodeGen/Mips/micromips-addiu.ll

 ; RUN: llc -march=mipsel -mcpu=mips32r2 -mattr=+micromips \
 ; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
+; For microMIPS64, also check 32 to 64 bit registers and 64 to 32 bit register copies
+; RUN: llc -march=mips -mcpu=mips64r6 -mattr=+micromips \
+; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
 
 @x = global i32 65504, align 4
 @y = global i32 60929, align 4
 @z = global i32 60929, align 4
 @.str = private unnamed_addr constant [7 x i8] c"%08x \0A\00", align 1
 
 define i32 @main() nounwind {
 entry:

test/CodeGen/Mips/micromips-andi.ll

 ; RUN: llc -march=mipsel -mcpu=mips32r2 -mattr=+micromips \
 ; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
+; For microMIPS64, also check 32 to 64 bit registers and 64 to 32 bit register copies
+; RUN: llc -march=mips -mcpu=mips64r6 -mattr=+micromips \
+; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
 
 @x = global i32 65504, align 4
 @y = global i32 60929, align 4
 @.str = private unnamed_addr constant [7 x i8] c"%08x \0A\00", align 1
 
 define i32 @main() nounwind {
 entry:
   %0 = load i32, i32* @x, align 4

test/CodeGen/Mips/micromips-gp-rc.ll

 ; RUN: llc -march=mipsel -mcpu=mips32r2 -mattr=+micromips \
 ; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
+; For microMIPS64, also check 32 to 64 bit registers and 64 to 32 bit register copies
+; RUN: llc -march=mips -mcpu=mips64r6 -mattr=+micromips \
+; RUN:   -relocation-model=pic -O3 < %s | FileCheck %s
 
 @g = external global i32
 
 ; Function Attrs: noreturn nounwind
 define void @foo() #0 {
 entry:
   %0 = load i32, i32* @g, align 4
   tail call void @exit(i32 signext %0)
   unreachable
 }
 
 ; Function Attrs: noreturn
 declare void @exit(i32 signext)
 
 ; CHECK: move $gp, ${{[0-9]+}}