[PATCH] D44176: [mips] Add support for CRC ASE.

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lib/Target/Mips/Mips.td:176
 
+def FeatureCRC : SubtargetFeature<"crc", "HasCRC", "true", "Mips (R6) CRC ASE">;
+
----------------
As the CRC ASE is only defined for R6, you can drop the brackets.


================
Comment at: lib/Target/Mips/Mips32r6InstrFormats.td:587-588
+  let Inst{20-16} = rt;
+  let Inst{15-9} = 0b0000000;
+  let Inst{8} = direction;
+  let Inst{7-6} = sz;
----------------
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?


================
Comment at: lib/Target/Mips/Mips32r6InstrInfo.td:949-955
+let AdditionalPredicates = [NotInMicroMips] in {
+  def CRC32B : R6MMR6Rel, CRC32B_ENC, CRC32B_DESC, ASE_CRC;
+  def CRC32H : R6MMR6Rel, CRC32H_ENC, CRC32H_DESC, ASE_CRC;
+  def CRC32W : R6MMR6Rel, CRC32W_ENC, CRC32W_DESC, ASE_CRC;
+  def CRC32CB : R6MMR6Rel, CRC32CB_ENC, CRC32CB_DESC, ASE_CRC;
+  def CRC32CH : R6MMR6Rel, CRC32CH_ENC, CRC32CH_DESC, ASE_CRC;
+  def CRC32CW : R6MMR6Rel, CRC32CW_ENC, CRC32CW_DESC, ASE_CRC;
----------------
This needs ISA_MIPS32R6 for these instructions. The ASE_CRC adjective comes after the ISA adjective.


================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:42
 class SCD_R6_ENC : SPECIAL3_LL_SC_FM<OPCODE6_SCD>;
+class CRC32D_ENC : SPECIAL3_2R_SZ_CRC<3,0>;
+class CRC32CD_ENC : SPECIAL3_2R_SZ_CRC<3,1>;
----------------
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.


================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:182-183
+let AdditionalPredicates = [NotInMicroMips] in {
+  def CRC32D : R6MMR6Rel, CRC32D_ENC, CRC32D_DESC, ASE_CRC64;
+  def CRC32CD : R6MMR6Rel, CRC32CD_ENC, CRC32CD_DESC, ASE_CRC64;
+}
----------------
This needs ISA_MIPS64R6 for these instructions, ISA adjectives come first, then the ASE adjectives.


================
Comment at: lib/Target/Mips/MipsInstrInfo.td:249-252
+def HasCRC   : Predicate<"Subtarget->hasCRC()">,
+               AssemblerPredicate<"FeatureCRC,FeatureMips32r6">;
+def HasCRC64 : Predicate<"Subtarget->hasCRC()">,
+               AssemblerPredicate<"FeatureCRC,FeatureMips64r6">;
----------------
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.


================
Comment at: test/MC/Disassembler/Mips/crc/valid-32r6-el.txt:2
+# RUN: llvm-mc --disassemble %s -triple=mipsel-unknown-linux -mcpu=mips32r6 \
+# RUN: -mattr=+crc | FileCheck %s
+0x0f 0x00 0x41 0x7c  # CHECK: crc32b $1, $2, $1
----------------
Indent the -mattr=+crc by two spaces here.


================
Comment at: test/MC/Disassembler/Mips/crc/valid-32r6.txt:2
+# RUN: llvm-mc --disassemble %s -triple=mips-unknown-linux -mcpu=mips32r6 \
+# RUN: -mattr=+crc | FileCheck %s
+0x7c 0x41 0x00 0x0f  # CHECK: crc32b $1, $2, $1
----------------
Indent the -mattr=+crc by two spaces here.


================
Comment at: test/MC/Disassembler/Mips/crc/valid-64r6-el.txt:2
+# RUN: llvm-mc --disassemble %s -triple=mipsel-unknown-linux -mcpu=mips64r6 \
+# RUN: -mattr=+crc | FileCheck %s
+0x0f 0x00 0x41 0x7c  # CHECK: crc32b $1, $2, $1
----------------
Indent the -mattr=+crc by two spaces here.


================
Comment at: test/MC/Disassembler/Mips/crc/valid-64r6.txt:2
+# RUN: llvm-mc --disassemble %s -triple=mips-unknown-linux -mcpu=mips64r6 \
+# RUN: -mattr=+crc | FileCheck %s
+0x7c 0x41 0x00 0x0f  # CHECK: crc32b $1, $2, $1
----------------
Indent the -mattr=+crc by two spaces here.


================
Comment at: test/MC/Mips/crc/invalid.s:1
+# Instructions that are invalid.
+#
----------------
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?


================
Comment at: test/MC/Mips/crc/module-crc.s:2
+# RUN: llvm-mc %s -triple=mips64-unknown-linux -mcpu=mips32r6 | \
+# RUN: FileCheck %s -check-prefix=CHECK-ASM
+#
----------------
Indent the FileCheck command here by two spaces.


================
Comment at: test/MC/Mips/crc/module-crc.s:5-7
+# RUN: -filetype=obj -o - | \
+# RUN: llvm-readobj -mips-abi-flags - | \
+# RUN: FileCheck %s -check-prefix=CHECK-OBJ
----------------
Indent these lines by two spaces.


Repository:
  rL LLVM

https://reviews.llvm.org/D44176