[PATCH] [mips] Correct and improve special-case shuffle instructions.

Vasileios Kalintiris Vasileios.Kalintiris at imgtec.com
Thu May 14 04:21:31 PDT 2015


LGTM with a couple nits/suggestions.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:2468-2469
@@ -2467,1 +2467,4 @@
 
+/// Determine whether a range fits a regular pattern of values.
+/// This function accounts for the possibility of jumping over the End iterator.
+template <typename ValType>
----------------
Did you intend to use 3 slashes instead of 2?

================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:2470-2475
@@ +2469,8 @@
+/// This function accounts for the possibility of jumping over the End iterator.
+template <typename ValType>
+static bool
+fitsRegularPattern(typename SmallVectorImpl<ValType>::const_iterator Begin,
+                   unsigned CheckStride,
+                   typename SmallVectorImpl<ValType>::const_iterator End,
+                   ValType ExpectedIndex, unsigned ExpectedIndexStride) {
+  auto &I = Begin;
----------------
  - Every instantiation of the template is using an `int`. There's no need for it.

  - Just a personal preference but can we swap the 2nd argument with the 3rd, so that `End` comes right after `Begin`?

  - Also, can we document a little bit the role of each argument and find a more descriptive name for ExpectedIndex?






================
Comment at: test/CodeGen/Mips/msa/shuffle.ll:4
@@ -3,2 +3,3 @@
 
 define void @vshf_v16i8_0(<16 x i8>* %c, <16 x i8>* %a, <16 x i8>* %b) nounwind {
+  ; CHECK-LABEL: vshf_v16i8_0:
----------------
The 3rd argument is unused in this and some of the following tests. Can we remove it from those function definitions while we're at it?

http://reviews.llvm.org/D9660

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list