[PATCH] D29623: [ARM] Replace HasT2ExtractPack with HasDSP

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

This looks mechanically sound to me, and I'm always in favor of removing subtarget features if we don't really need them. That being said, I'd appreciate it if someone that knows more about these extensions gave the final ok.



================
Comment at: lib/Target/ARM/ARMInstrThumb2.td:1145
+
+// UXTB16, SXTB16 - Requires T2ExtractPack, does not need the .w qualifier.
+class T2I_ext_rrot_xtb16<bits<3> opcod, string opc>
----------------
You mean Requires DSP, right?


================
Comment at: test/CodeGen/Thumb2/thumb2-sxt-uxt.ll:39
 ; CHECK-M4: sxtab r0, r0, r1
+; CHECK-ARMv7EM: sxtab r0, r0, r1
+; CHECK-ARMv8M-DSP: sxtab r0, r0, r1
----------------
You should share the same check prefixes for the similar cases, e.g. CHECK-NO-DSP for cortex-m3 and armv8m, and CHECK-DSP for the others. Then you don't have to repeat yourself in the checks :)

Same for the other test files.


================
Comment at: test/CodeGen/Thumb2/thumb2-sxt-uxt.ll:41
+; CHECK-ARMv8M-DSP: sxtab r0, r0, r1
+; CHECK-ARMv8M-NOT: sxtab r0, r0, r1
   %sext = sext i8 %b to i32
----------------
This is too tight, I think it's better to use CHECK-NOT: sxtab (like you did in the other test files) since you don't want to see sxtab _at all_ (the registers don't matter).


================
Comment at: test/CodeGen/Thumb2/thumb2-uxt_rot.ll:29
+; ARMv8M: uxtb r1, r1
+; ARMv8M-NOT: uxtab  r0, r0, r1
+; ARMv8M: add r0, r1
----------------
Same as above, get rid of the registers here.


================
Comment at: test/CodeGen/Thumb2/thumb2-uxtb.ll:12
+; ARMv7EM: uxtb16 r0, r0
+; ARMv8M-NOT: uxtb16 r0, r0
+; ARMv8M-DSP: uxtb16 r0, r0
----------------
Same as above (in the whole file)


https://reviews.llvm.org/D29623