[PATCH] D17068: [mips][microMIPS] Fix for "Cannot copy registers" assertion

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: test/CodeGen/Mips/invalid_operands.ll:1-23
@@ +1,23 @@
+; 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)
----------------
hvarga wrote:
> vkalintiris wrote:
> > 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. 
> 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.
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.

================
Comment at: test/CodeGen/Mips/micromips-addiu.ll:3
@@ -2,1 +2,3 @@
 ; 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 \
----------------
hvarga wrote:
> vkalintiris wrote:
> > 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?
> 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.
>  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.


http://reviews.llvm.org/D17068