[PATCH] D11678: [CodeGen] Fixes *absdiff* intrinsic: LangRef doc/test case improvement and corresponding code change

[Ahmed Bougacha via llvm-commits]

ab added a subscriber: ab.
ab added a comment.

I'm not a fan of the way the tests are written: they seem both too brittle and too strict.

FWIW, I'd start by using utils/update_llc_test_checks.py, and then refining away the non-ABI-specified registers into regexes.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:726
@@ -725,2 +725,3 @@
   SDLoc dl(Op);
-  SDValue Tmp1, Tmp2, Tmp3, Tmp4;
+  SDValue Sub, Neg, Cmp;
+  SDValue Op0 = Op.getOperand(0);
----------------
Can you define these at the point of initialization instead?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:735
@@ +734,3 @@
+  unsigned NumElt = VT.getVectorNumElements();
+  MVT NVT;
+  if (isUabsdiff) {
----------------
Again, please initialize variables when defining them.

Given line 745, NVT is unnecessary, I think. The block at l736 could just set VT instead.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:737-738
@@ +736,4 @@
+  if (isUabsdiff) {
+    NVT = TLI.getTypeToPromoteTo(ISD::UABSDIFF, EltVT.getSimpleVT());
+    NVT = MVT::getVectorVT(NVT, NumElt);
+    Op0 = DAG.getNode(ISD::ZERO_EXTEND, dl, NVT, Op0);
----------------
This logic is fishy, IMO: it doesn't make sense for targets to set the element type promotion rule, and asking for the promoted vector type might get us a widened version.

What about using:

    VT.widenIntegerVectorElementType(*DAG.getContext());

which sounds like what you want, given that there's no other sensible thing the target could do.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:318-323
@@ -317,2 +317,8 @@
   // encoding.
+  setOperationAction(ISD::UABSDIFF         , MVT::i8   , Promote);
+  AddPromotedToType (ISD::UABSDIFF         , MVT::i8   , MVT::i16);
+  setOperationAction(ISD::UABSDIFF         , MVT::i16  , Promote);
+  AddPromotedToType (ISD::UABSDIFF         , MVT::i16  , MVT::i32);
+  setOperationAction(ISD::UABSDIFF         , MVT::i32  , Promote);
+  AddPromotedToType (ISD::UABSDIFF         , MVT::i32  , MVT::i64);
   setOperationAction(ISD::CTTZ             , MVT::i8   , Promote);
----------------
This will become unnecessary if you use EVT::widenIntegerVectorElementType above.

================
Comment at: test/CodeGen/X86/absdiff_128.ll:11-12
@@ +10,4 @@
+; CHECK:      punpckldq
+; CHECK-DAG:  movd   %xmm1, [[SRC:%e[a-d]+x]]
+; CHECK-DAG:  movd   %xmm0, [[DST:%e[a-d]+x]]
+; CHECK:      subl [[SRC]], [[DST]]
----------------
The a-d seems too restrictive (what about, say, %r9d?), and '+' seems unnecessary.

================
Comment at: test/CodeGen/X86/absdiff_256.ll:5
@@ +4,3 @@
+
+define <16 x i32> @test_sabsdiff_v16i32_expand(<16 x i32> %a1, <16 x i32> %a2) {
+; CHECK-LABEL: test_sabsdiff_v16i32_expand
----------------
Why test v16i32?  IMHO the output is huge without providing more coverage.

Should this be v16i16 or something?


http://reviews.llvm.org/D11678