This is an archive of the discontinued LLVM Phabricator instance.

[mips] Properly select abs and sqrt instructions
ClosedPublic

Authored by smaksimovic on Dec 20 2017, 6:02 AM.

Details

Summary
  • Alter abs for micromips to have both AFGR64 and FGR64 variants, same as sqrt
  • Remove sqrt and abs from MicroMips32r6InstrInfo.td, use micromips FGR64 variants
  • Restrict non-micromips abs/sqrt with NotInMicroMips predicate

Diff Detail

Repository
rL LLVM

Event Timeline

smaksimovic created this revision.Dec 20 2017, 6:02 AM

This looks

lib/Target/Mips/MicroMips32r6InstrInfo.td
1627 ↗(On Diff #127697)

Spaces around the '='.

1655 ↗(On Diff #127697)

Here too.

lib/Target/Mips/MicroMipsInstrFPU.td
96 ↗(On Diff #127697)

Nit, align the "SDPatternOperator [...]" to under the 'string' in the line above, spaces around the '='.

101 ↗(On Diff #127697)

FIXME: This needs to be part of the instruction mapping tables.

197 ↗(On Diff #127697)

Nit: the closing '}' should be aligned with the def or class it's apart of.

lib/Target/Mips/MipsInstrFPU.td
462–464 ↗(On Diff #127697)

Nit, indent this by 2 columns.

test/CodeGen/Mips/abs_sqrt.ll
1 ↗(On Diff #127697)

Modify the existing file test/CodeGen/Mips/llvm-ir/sqrt.ll for the square root tests.

Add test/CodeGen/Mips/llvm-ir/abs.ll for the abs tests.

2 ↗(On Diff #127697)

cpu should be mips32r2.

3–4 ↗(On Diff #127697)

cpu should be mips32r3.

test/MC/Disassembler/Mips/micromips32/abs_sqrt.txt
1 ↗(On Diff #127697)

This should go in test/MC/Disassembler/Mips/micromips32r3/valid.txt and the little endian disassembly in test/MC/Disassembler/Mips/micromips32r3/valid-el.txt

test/MC/Disassembler/Mips/micromips32/abs_sqrt_fp64.txt
1 ↗(On Diff #127697)

This should go in test/MC/Disassembler/Mips/micromips32r3/valid-fp64.txt and provide little endian disassembly in test/MC/Disassembler/Mips/micromips32r3/valid-fp64-el.txt.

The -mcpu= should be mips32r3. You should drop the explicit check prefix and use CHECK: for the check lines instead. Also keep the hex and CHECK on the same line, i.e.

0x54 0x0c 0x0a 0x3b
# MMFP64:     sqrt.s  $f0, $f12

Should be:

0x54 0x0c 0x0a 0x3b # CHECK: sqrt.s  $f0, $f12
test/MC/Disassembler/Mips/mips32/abs_sqrt.txt
1 ↗(On Diff #127697)

This file can be entirely dropped, existing disassembly tests cover it.

test/MC/Disassembler/Mips/mips32/abs_sqrt_fp64.txt
1 ↗(On Diff #127697)

Rename this file to valid-fp64.txt and place copies in test/MC/Disassembler/Mips/mips32r{2, 3, 5}/

Also provide little endian disassembly files. You can drop the explicit check prefix and just use 'CHECK'. See our existing disassembly files and copy their style.

test/MC/Disassembler/Mips/mips32r6/abs_sqrt.txt
1 ↗(On Diff #127697)

This file can be dropped as the existing tests cover it.

test/MC/Mips/abs_sqrt.s
1 ↗(On Diff #127697)

Rather than providing this as a new test, you can modify existing tests to cover almost all the variants here.

For the mips32 case, modify or ensure that test/MC/Mips/mips{1, 2, 32, 32r2, 32r3, 32r5}/valid.s produce the non-micromips AFGR64 instruction. If you look at the 'abs.d' case in test/MC/Mips/micromips/valid.s, you'll see how it should look.

I've highlighted the additional changes for this file.

2 ↗(On Diff #127697)

Provide this variant in test/MC/Mips/mips32r{2,3,5}/valid-fp64.s. The cpu should correspond to the directory it's in. Also test which instruction the assembly has determined it is with -show-inst.

3 ↗(On Diff #127697)

Check which instructions are produced from test/MC/Mips/micromips/valid.s with the -show-inst option.

4 ↗(On Diff #127697)

Provide this variant in test/MC/Mips/micromips/valid-fp64.s. Also the cpu should be mips32r3.

5 ↗(On Diff #127697)

Check which instructions are produced from test/MC/Mips/micromips32r6/valid.s with the -show-inst option.

28 ↗(On Diff #127697)

This case is already covered as you've modified test/MC/Mips/micromips/valid.s

Addressed review comments.

I made a rather inconvenient naming change to the MMR6 version of SQRT.
The SQRT_D_MMR6, as it was originally named, came after FSQRT_D64_MM in the match table residing in MipsGenAsmMatcher.inc.
This was not the case with ABS, so it seems that the instructions are ordered by name as well, among other things.
Renamed SQRT_D_MMR6 to ASQRT_D_MMR6. I didn't find another way to force the ordering of records in the match table.

This looks

lib/Target/Mips/MicroMips32r6InstrInfo.td
1627–1630 ↗(On Diff #129261)

You can remove the definitions of these instructions from MicroMips32r6 as you have provided the FGR64 definitions in MicroMipsInstrFPU.td and the instructions share the same encodings.

1655–1660 ↗(On Diff #129261)

I've checked this again, you can remove the definition of these instructions from MicroMips32r6InstrInfo.td, as they share the same encoding as the original microMIPS instruction and you have provided the FGR64 definitions in MicroMipsInstrFPU.td.

smaksimovic edited the summary of this revision. (Show Details)

Removed instructions from MicroMips32r6InstrInfo.td

sdardis accepted this revision.Jan 22 2018, 6:36 AM

LGTM.

This revision is now accepted and ready to land.Jan 22 2018, 6:36 AM
This revision was automatically updated to reflect the committed changes.