This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement CFC*, CTC* and LDC* instructions
ClosedPublic

Authored by hvarga on Apr 29 2016, 1:27 AM.

Details

Summary

This patch implements microMIPS32r6 CFC1, CFC2, CTC1, CTC2, LDC1 and LDC2 instructions.

There was a problem with the previous implementation of this patch (D18640) and because of that commit rL266861 was reverted.
After committing of the previous patch, test-suite failed with error message in the form of:

fatal error: error in backend: Cannot select: 0x3b52390: f64,ch = load<LD8[%43](tbaa=<0x3a0db98>)> 0x3af1180, 0x3b50300, undef:i32

There was a problem with selecting LDC1 instruction in LLVM backend.

For that reason, it is decided to revert commit rL266861 and make this patch which besides implementation of instructions and standard regression tests also includes CodeGen test (ldc1.ll).

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 55543.Apr 29 2016, 1:27 AM
hvarga retitled this revision from to [mips][microMIPS] Implement CFC*, CTC* and LDC* instructions.
hvarga updated this object.
hvarga added subscribers: llvm-commits, petarj, sdardis, dsanders.
sdardis requested changes to this revision.May 4 2016, 3:54 AM
sdardis edited edge metadata.

Compiling test/CodeGen/Mips/ldc1.ll with "-mcpu=mips32r6 -matt=+micromips -asm-show-inst" shows the correct assembly but incorrect internal representation:

ldc1    $f0, 0($4)              # <MCInst #1184 LDC1_D64_MMR6
                                #  <MCOperand Reg:361>
                                #  <MCOperand Reg:22>
                                #  <MCOperand Imm:0>>
sll16   $2, $2, 3               # <MCInst #1952 SLL16_MM
                                #  <MCOperand Reg:321>
                                #  <MCOperand Reg:321>
                                #  <MCOperand Imm:3>>
ldc1    $f1, 0($sp)             # <MCInst #1182 LDC164 <- wrong
                                #  <MCOperand Reg:362>
                                #  <MCOperand Reg:20>
                                #  <MCOperand Imm:0>>

This hampers direct object emission as ldc1 has a different major opcodes for microMIPS/MIPS. It appears the backend code doesn't have the correct entries in the relation tables to map LDC164 to LDC1_D64_MMR6, so reloads are incorrect.

Can you get that relationship entry implemented and repost? Thanks.

This revision now requires changes to proceed.May 4 2016, 3:54 AM
hvarga updated this revision to Diff 56243.EditedMay 5 2016, 1:06 AM
hvarga edited edge metadata.

Fixed selection of LDC164 over LDC1_D64_MMR6 definition in the case of microMIPS32r6.

This was happening because of a missing pattern for load instructions with a reg+imm operand in lib/Target/Mips/MicroMips32r6InstrInfo.td.

sdardis requested changes to this revision.May 5 2016, 3:27 AM
sdardis edited edge metadata.

Looking at the -asm-show-inst output of the given test shows that that the latest change does introduce LDC1_64_MMR6 for all uses in ldc1.ll.

I checked test/CodeGen/Mips/cconv/callee-saved-float.ll with -asm-show-inst and I'm still seeing LDC164 being used for reloading values from the stack. We need the appropriate mapping between LDC164 and LDC1_64_MMR6.

This revision now requires changes to proceed.May 5 2016, 3:27 AM
hvarga updated this revision to Diff 56553.May 9 2016, 4:50 AM
hvarga edited edge metadata.

Fixed mapping between LDC164 and LDC1_64_MMR6. I have also extended the test test/CodeGen/Mips/cconv/callee-saved-float.ll to check whether the LDC1_64_MMR6 is used in case of microMIPS32r6.

sdardis accepted this revision.May 10 2016, 2:19 AM
sdardis edited edge metadata.

LGTM with nits (inlined) addressed.

test/CodeGen/Mips/cconv/callee-saved-float.ll
20 ↗(On Diff #56553)

Add

; CHECK-NOT: LDC164

along with -asm-show-inst

113 ↗(On Diff #56553)

Nit: Insert a blank line between the existing and new checks, so that it's very obvious it's testing something different.

test/CodeGen/Mips/ldc1.ll
4–58 ↗(On Diff #56553)

All of this can be simplified to:

define double @f(double * %a){
  ; CHECK-LABEL: f
  ; CHECK:     ldc1
  ; CHECK-NOT: LDC164
  %1 = load double, double * %a
  ret double %1
}

Checking spills and reloads is covered by callee-saved-float.ll

This revision is now accepted and ready to land.May 10 2016, 2:19 AM
hvarga added inline comments.May 10 2016, 11:21 PM
test/CodeGen/Mips/cconv/callee-saved-float.ll
20 ↗(On Diff #56553)

I can not check that LDC164 is not occurring in this test file because it will occur. :) So, I can not use -asm-show-inst to check for correct mapping.

But this is not a problem since LDC164 is indeed correctly mapped to LDC1_64_MMR6. This is tested by generating the object file with llc and afterwards dumping it using llvm-objdump. This output is then checked for the occurrence of ldc1. From my perspective, since the object file is compiled and decompiled as microMIPS32r6 (containing ldc1 in its output), I think that this also proves that the correct implementation of ldc1 is selected.

Do you agree, can I commit this patch into mainline?

sdardis added inline comments.May 11 2016, 2:40 AM
test/CodeGen/Mips/cconv/callee-saved-float.ll
20 ↗(On Diff #56553)

Yes, I do. Ignore what I said about -asm-show-inst.

This revision was automatically updated to reflect the committed changes.