This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add support for CRC ASE
ClosedPublic

Authored by vstefanovic on Mar 6 2018, 2:13 PM.

Details

Summary

Instructions: crc32b, crc32h, crc32w, crc32d,

crc32cb, crc32ch, crc32cw, crc32cd

Assembler directives: .set crc, .set nocrc, .module crc, .module nocrc
Attribute: crc
.MIPS.abiflags: CRC (0x8000)

Diff Detail

Repository
rL LLVM

Event Timeline

vstefanovic created this revision.Mar 6 2018, 2:13 PM
sdardis requested changes to this revision.Mar 7 2018, 7:46 AM

This looks mostly ok. There's only some small changes required, and they are somewhat minor. The recurring change is that for test cases, when there is a run-on line with '\', then the continuation of the command line should be indented by two spaces.

See my inlined comments.

lib/Target/Mips/Mips.td
179

As the CRC ASE is only defined for R6, you can drop the brackets.

lib/Target/Mips/Mips32r6InstrFormats.td
587–588

Rather than "simplifying" the encoding layout, can you transcribe it directly so that bits 15 to 11 are zeros, and the direction field is a three bit parameter?

lib/Target/Mips/Mips32r6InstrInfo.td
949–955

This needs ISA_MIPS32R6 for these instructions. The ASE_CRC adjective comes after the ISA adjective.

lib/Target/Mips/Mips64r6InstrInfo.td
42

Nit: align the ':' of this line with the one below. It will look a bit odd w.r.t. the line above, but most of these lines should align w.r.t. ';', but that's a cosmetic nit for another day.

184–185

This needs ISA_MIPS64R6 for these instructions, ISA adjectives come first, then the ASE adjectives.

lib/Target/Mips/MipsInstrInfo.td
249–252

Can you remove FeatureMipsXXr6 from these assembler predicates? It's my view that ASE and ISA level adjectives/predicates should be separated, such that both can be applied to an instruction without overlap.

test/MC/Disassembler/Mips/crc/valid-32r6-el.txt
3

Indent the -mattr=+crc by two spaces here.

test/MC/Disassembler/Mips/crc/valid-32r6.txt
3

Indent the -mattr=+crc by two spaces here.

test/MC/Disassembler/Mips/crc/valid-64r6-el.txt
3

Indent the -mattr=+crc by two spaces here.

test/MC/Disassembler/Mips/crc/valid-64r6.txt
3

Indent the -mattr=+crc by two spaces here.

test/MC/Mips/crc/invalid.s
2

Can you also add an invalid-wrong-isa.s test file which uses .set mipsXX * .set micromips to show that we correctly reject the instruction for pre-R6 ISAs?

test/MC/Mips/crc/module-crc.s
3

Indent the FileCheck command here by two spaces.

6–8

Indent these lines by two spaces.

This revision now requires changes to proceed.Mar 7 2018, 7:46 AM
vstefanovic added inline comments.Mar 8 2018, 6:55 AM
lib/Target/Mips/Mips32r6InstrInfo.td
949–955

This:

def CRC32CW : R6MMR6Rel, CRC32CW_ENC, CRC32CW_DESC, ISA_MIPS32R6, ASE_CRC;

ends up being the same as this:

def CRC32CW : R6MMR6Rel, CRC32CW_ENC, CRC32CW_DESC, ASE_CRC;

because (I think) ISA_MIPS32R6's InsnPredicates get overridden by ASE_CRC's InsnPredicates.
We could do:

class ASE_CRC {
  list <Predicate> InsnPredicates = [HasCRC, HasMips32r6];
}

Btw, we might want to improve this with other ASEs too (in a separate patch), for example this MT case goes without a warning or error:

.set mt
.set mips2
yield $4
sdardis added inline comments.Mar 8 2018, 7:47 AM
lib/Target/Mips/Mips32r6InstrInfo.td
949–955

Good point. I have a posted patch which includes creating an additional field in PredicateControl which seperates ASEs from the other instruction predicates.

I'll factor it out of that patch and include some other refactoring I have in my local copy and commit that.

If we're getting no error from that assembly fragment you've posted, we can solve that in general and you can disregard my requested change for testing that .set correctly rejects the crc instructions.

vstefanovic retitled this revision from [mips] Add support for CRC ASE. to [mips] Add support for CRC ASE.
vstefanovic marked 15 inline comments as done.Mar 8 2018, 11:57 AM
vstefanovic added inline comments.
test/MC/Mips/crc/invalid.s
2

I have invalid-wrong-isa.s locally now, if we want to add it later.

If you look at the dependency, you'll see the way I've separated out the ISA level from the ASEs.

vstefanovic marked an inline comment as done.

Updated the code to use ASEPRedicate from D44299.

sdardis accepted this revision.Mar 13 2018, 4:14 AM

LGTM with inline nits addressed.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1136

Newline before and after what you've added here.

lib/Target/Mips/Mips64r6InstrInfo.td
119

Nit: New line before and after this hunk.

test/MC/Disassembler/Mips/crc/valid-64r6-el.txt
2

Triple here should be mips64el-unknown-linux-gnu.

test/MC/Disassembler/Mips/crc/valid-64r6.txt
2

Triple here should be mips64-unknown-linux-gnu.

test/MC/Mips/crc/invalid64.s
4

Triple here should be mips64-unknown-linux-gnu.

test/MC/Mips/crc/module-crc.s
2

Triple here should be mips-unknown-linux-gnu.

5

Triple here should be mips-unknown-linux-gnu.

test/MC/Mips/crc/set-nocrc-directive.s
5

Triple here should be mips64-unknown-linux-gnu.

test/MC/Mips/crc/valid.s
4

Triple here should be mips64-unknown-linux-gnu.

test/MC/Mips/crc/valid64.s
2

Triple here should be mips64-unknown-linux-gnu.

This revision is now accepted and ready to land.Mar 13 2018, 4:14 AM

Addressed nits.

vstefanovic marked 11 inline comments as done.Mar 13 2018, 11:42 AM
This revision was automatically updated to reflect the committed changes.