Page MenuHomePhabricator

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

Authored by hvarga on Feb 10 2016, 4:12 AM.

Details

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.

Diff Detail

Event Timeline

hvarga updated this revision to Diff 47438.Feb 10 2016, 4:12 AM
hvarga retitled this revision from to [mips][microMIPS] Fix for "Cannot copy registers" assertion.
hvarga updated this object.
hvarga added subscribers: llvm-commits, petarj.
sdardis edited edge metadata.

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?

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

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.

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 updated this revision to Diff 48660.Feb 22 2016, 2:40 AM
hvarga edited edge metadata.

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

I will wait until D16220 is committed.

vkalintiris edited edge metadata.Mar 8 2016, 9:03 AM

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

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 added a comment.EditedMar 15 2016, 6:06 AM

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.

This revision was automatically updated to reflect the committed changes.
hvarga updated this revision to Diff 53196.EditedApr 11 2016, 12:17 AM
hvarga edited edge metadata.
hvarga removed rL LLVM as the repository for this revision.

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.

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.

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 marked 3 inline comments as done.Apr 11 2016, 11:32 PM
hvarga added inline comments.
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 added inline comments.Apr 12 2016, 2:50 AM
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 marked 3 inline comments as done.Apr 12 2016, 3:58 AM
hvarga added inline comments.
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.