[PowerPC] fix incorrect vectorization of abs() on POWER9

[inouehrs]

Summary

Vectorized loops with abs() returns incorrect results on POWER9. This patch fixes it.
For example the following code returns negative result if input values are negative though it sums up the absolute value of the inputs. This problem causes test failures for libvpx.

int vpx_satd_c(const int16_t *coeff, int length) {
  int satd = 0;
  for (int i = 0; i < length; ++i) satd += abs(coeff[i]);
  return satd;
}

For vector absolute and vector absolute difference on POWER9, LLVM generates VABSDUW (Vector Absolute Difference Unsigned Word) instruction or variants.
Since these instructions are for unsigned integers, we need adjustment for signed integers.
For abs(sub(a, b)), we generate VABSDUW(a+0x80000000, b+0x80000000). Otherwise, abs(sub(-1, 0)) returns 0xFFFFFFFF(=-1) instead of 1. For abs(a), we generate VABSDUW(a+0x80000000, 0x80000000).

[inouehrs]

• fix for halfword and byte cases

[nemanjai]

I imagine the constant materialization and splatting direct moves would not cost anything additional to what I suggested in an amortized sense (i.e. if LICM takes them out of the loop). However, I think it's still useful to produce code with lower path length.

Also, I can't really think of a better way to do it for the halfword version. I imagine there isn't a better way.
Finally for consistency, I would just use vxor instead of the various add opcodes. I think that is semantically equivalent to the modulo adds (correct me if I'm wrong here).

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4811

Shouldn't this check ZERO_EXTEND_VECTOR_INREG as well? Or is that a node we can't have this late?

4816

It seems that for the v4i32 type, we should be able to just use xvnegsp rather than loading the immediate, moving and adding.

4834

We should just be able to do something like:

xxspltib 35, 128 # Mask
vxor 0, 3, 2    # Flip sign
vabsduh ...      # The actual absdiff

[inouehrs]

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4811

In my understanding ZERO_EXTEND_VECTOR_INREG is created in the legalize phase, while this code is for the initial selection phase. So I think this code will not find ZERO_EXTEND_VECTOR_INREG node here.

4816

Do you know it is safe to use a floating point instruction for integer data if the bit pattern is for NaN of Inf?

4834

Good catch. I will update to use xxspltib.
VSX splat immediate supports 8-bit immediate while older VMX splat immediate supports only 5 bits.

[nemanjai]

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4816

I think it's OK according to the ISA since it doesn't modify any special registers or do anything special for NaN/Inf. The description just says that it copies the contents with the high bit of each word element complemented, so I think this is just a bitwise operation rather than a vector fp operation.

[inouehrs]

addressed comments from @nemanjai

[inouehrs]

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

4816

As far as I tested, it works at least on POWER9.

[nemanjai]

Sorry, I find the code kind of difficult to follow now. This is exacerbated by the fact that we end up creating nodes with AddOpcode that produce unary operations - which is very counter intuitive. I think it would be much easier to follow if you made it flow more naturally. Perhaps something along the lines of:

MachineSDNode *flipSign(...)
  if (Type == v4i32)
    <produce and return XVNEGSP>
  if (Type == v8i16)
    <produce the { 0x8000, 0x8000, ... } vector> // The implicit CSE in the DAG will ensure we don't get multiple nodes
  else if (Type == v16i8)
    <produce the { 0x80, 0x80, ... } vector>     // The implicit CSE in the DAG will ensure we don't get multiple nodes
  if (InputOp == Zero)
    <return vector from above>

  <produce and return the add/xor>
...
if (SkipAdjust)
  <just produce VABSDU[BHW], replace, return>
if (Opcode == SUB)
  Op1 = flipSign(Operand1)
  Op2 = flipSign(Operand2)
else
  Op1 = flipSign(Operand1)
  Op2 = flipSign(Zero)
<produce VABSDU[BHW] with Op1/Op2, replace, return>

I think that if it's structured in a similar way, it is much easier to follow the code and see exactly what is going on.

[inouehrs]

• refactoring based on the suggestion from @nemanjai

[nemanjai]

I think this flows much more cleanly now. And if you reorder it to not create the bool temporary and return early, I think it'll be even better. Feel free to do that on the commit, I don't think this needs another review cycle.

BTW. Using V_SET0 or XXLXORz instead of XXSPLTIB to produce a zero vector might be a better choice since it has a 1-cycle lower maximum latency (but in practice, this shouldn't be noticeable).

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

336

This is a member function and you're passing CurDAG which is a data member of the same class, right? If so, please remove the argument. Also, the SDLoc is easy enough to get from N.

3981

Since you're setting the output parameter rather than returning it...
s/returns/sets it to

3996

I suppose we could do this by:

vspltish 5, 1
vspltish 6, 15
vslh 5, 6, 5

As the two splats can be done in parallel. But this might very well be worse since it uses an extra vector register and due to the dispatch rules for vector operations.
Up to you of course.

4808

Can you just move this condition below where you set the opcodes and then we don't need SkipAdjust (since we now use it in only one place)?

4822

Can you please return here so the remaining code does not need to be nested into an else?

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

bool runOnMachineFunction(MachineFunction &MF) override {
         InsertVRSaveCode(MF);
 
       return true;
     }
 
     void PreprocessISelDAG() override;
     void PostprocessISelDAG() override;
 
+    /// getI8Imm - Return a target constant with the specified value, of type
+    /// i8.
+    inline SDValue getI8Imm(unsigned Imm, const SDLoc &dl) {
+      return CurDAG->getTargetConstant(Imm, dl, MVT::i8);
+    }
+
     /// getI16Imm - Return a target constant with the specified value, of type
     /// i16.
     inline SDValue getI16Imm(unsigned Imm, const SDLoc &dl) {
       return CurDAG->getTargetConstant(Imm, dl, MVT::i16);
     }
 
     /// getI32Imm - Return a target constant with the specified value, of type
     /// i32.
private:
     SDValue combineToCMPB(SDNode *N);
     void foldBoolExts(SDValue &Res, SDNode *&N);
 
     bool AllUsersSelectZero(SDNode *N);
     void SwapAllSelectUsers(SDNode *N);
 
     bool isOffsetMultipleOf(SDNode *N, unsigned Val) const;
     void transferMemOperands(SDNode *N, SDNode *Result);
   };
 
 } // end anonymous namespace
 
 /// InsertVRSaveCode - Once the entire function has been instruction selected,
 /// all virtual registers are created and all machine instructions are built,
 /// check to see if we need to save/restore VRSAVE.  If so, do it.
 void PPCDAGToDAGISel::InsertVRSaveCode(MachineFunction &Fn) {
   // Check to see if this function uses vector registers, which means we have to
void PPCDAGToDAGISel::transferMemOperands(SDNode *N, SDNode *Result) {
   // Transfer memoperands.
   MachineSDNode::mmo_iterator MemOp = MF->allocateMemRefsArray(1);
   MemOp[0] = cast<MemSDNode>(N)->getMemOperand();
   cast<MachineSDNode>(Result)->setMemRefs(MemOp, MemOp + 1);
 }
 
 // Select - Convert the specified operand from a target-independent to a
 // target-specific node if it hasn't already been changed.
 void PPCDAGToDAGISel::Select(SDNode *N) {
   SDLoc dl(N);
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);
     return;   // Already selected.
   }
 
   // In case any misguided DAG-level optimizations form an ADD with a
   // TargetConstant operand, crash here instead of miscompiling (by selecting
   // an r+r add instead of some kind of r+i add).
   if (N->getOpcode() == ISD::ADD &&
       N->getOperand(1).getOpcode() == ISD::TargetConstant)
     llvm_unreachable("Invalid ADD with TargetConstant operand");
 
   // Try matching complex bit permutations before doing anything else.
   if (tryBitPermutation(N))
     return;
 
   // Try to emit integer compares as GPR-only sequences (i.e. no use of CR).
   if (tryIntCompareInGPR(N))
     return;
 
   switch (N->getOpcode()) {
   default: break;
if ((Elt & 1) == 0) {
       SDNode *Tmp1 = CurDAG->getMachineNode(Opc1, dl, VT, EltVal);
       EltVal = getI32Imm(-16, dl);
       SDNode *Tmp2 = CurDAG->getMachineNode(Opc1, dl, VT, EltVal);
       ReplaceNode(N, CurDAG->getMachineNode(Opc2, dl, VT, SDValue(Tmp1, 0),
                                             SDValue(Tmp2, 0)));
       return;
     }
   }
+  case ISD::ABS: {
+    assert(PPCSubTarget->hasP9Vector() && "ABS is supported only with P9 Vector");
+
+    // For vector absolute difference, we use VABSDUW instruction of POWER9.
+    // Since VABSDU instructions are for unsigned integers, we need adjustment
+    // for signed integers.
+    // For abs(sub(a, b)), we generate VABSDUW(a+0x80000000, b+0x80000000).
+    // Otherwise, abs(sub(-1, 0)) returns 0xFFFFFFFF(=-1) instead of 1.
+    // For abs(a), we generate VABSDUW(a+0x80000000, 0x80000000).
+
+    EVT VecVT = N->getOperand(0).getValueType();
+    SDNode *ScaImm = nullptr;
+    SDNode *AbsOp = nullptr;
+    unsigned AbsOpcode, AddOpcode;
+
+    // Even for signed integers, we can skip adjustment if all values are
+    // known to be positive (as signed integer) due to zero-extended inputs.
+    bool SkipAdjust = false;
+    if (N->getOperand(0).getOpcode() == ISD::SUB) {
+      if (N->getOperand(0)->getOperand(0).getOpcode() == ISD::ZERO_EXTEND &&
+          N->getOperand(0)->getOperand(1).getOpcode() == ISD::ZERO_EXTEND)
+        SkipAdjust = true;
+    }
+
+    if (VecVT == MVT::v4i32) {
+      AbsOpcode = PPC::VABSDUW;
+      AddOpcode = PPC::VADDUWM;
+      if (!SkipAdjust)
+        ScaImm = CurDAG->getMachineNode(PPC::LIS, dl, MVT::i32,
+                                        getI32Imm(0x8000, dl));
+    }
+    else if (VecVT == MVT::v8i16) {
+      AbsOpcode = PPC::VABSDUH;
+      AddOpcode = PPC::VADDUHM;
+      if (!SkipAdjust)
+        ScaImm = CurDAG->getMachineNode(PPC::LI, dl, MVT::i16,
+                                        getI16Imm(0x8000, dl));
+    }
+    else if (VecVT == MVT::v16i8) {
+      AbsOpcode = PPC::VABSDUB;
+      AddOpcode = PPC::VADDUBM;
+      if (!SkipAdjust)
+        ScaImm = CurDAG->getMachineNode(PPC::LI, dl, MVT::i8,
+                                        getI8Imm(0x80, dl));
+    }
+    else
+      llvm_unreachable("Unsupported vector data type for ISD::ABS");
+
+    if (SkipAdjust)
+      AbsOp = CurDAG->getMachineNode(AbsOpcode, dl, VecVT,
+                                     SDValue(N->getOperand(0)->getOperand(0)),
+                                     SDValue(N->getOperand(0)->getOperand(1)));
+    else {
+      SDNode *VecImm = CurDAG->getMachineNode(PPC::MTVSRWS, dl, VecVT,
+                                              SDValue(ScaImm, 0));
+      if (N->getOperand(0).getOpcode() == ISD::SUB) {
+        SDValue SubVal = N->getOperand(0);
+        SDNode *Add0Op = CurDAG->getMachineNode(AddOpcode, dl, VecVT,
+                                                SDValue(SubVal->getOperand(0)),
+                                                SDValue(VecImm, 0));
+        SDNode *Add1Op = CurDAG->getMachineNode(AddOpcode, dl, VecVT,
+                                                SDValue(SubVal->getOperand(1)),
+                                                SDValue(VecImm, 0));
+        AbsOp = CurDAG->getMachineNode(AbsOpcode, dl, VecVT,
+                                       SDValue(Add0Op, 0), SDValue(Add1Op, 0));
+      }
+      else {
+        SDNode *AddOp = CurDAG->getMachineNode(AddOpcode, dl, VecVT,
+                                               SDValue(N->getOperand(0)),
+                                               SDValue(VecImm, 0));
+        AbsOp = CurDAG->getMachineNode(AbsOpcode, dl, VecVT, SDValue(AddOp, 0),
+                                       SDValue(VecImm, 0));
+      }
+    }
+    ReplaceNode(N, AbsOp);
+    return;
+  }
   }
 
   SelectCode(N);
 }
 
 // If the target supports the cmpb instruction, do the idiom recognition here.
 // We don't do this as a DAG combine because we don't want to do it as nodes
 // are being combined (because we might miss part of the eventual idiom). We

lib/Target/PowerPC/PPCInstrAltivec.td

def VABSDUB : VXForm_1<1027, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
                        [(set v16i8:$vD, (int_ppc_altivec_vabsdub v16i8:$vA, v16i8:$vB))]>;
 def VABSDUH : VXForm_1<1091, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
                        "vabsduh $vD, $vA, $vB", IIC_VecGeneral,
                        [(set v8i16:$vD, (int_ppc_altivec_vabsduh v8i16:$vA, v8i16:$vB))]>;
 def VABSDUW : VXForm_1<1155, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
                        "vabsduw $vD, $vA, $vB", IIC_VecGeneral,
                        [(set v4i32:$vD, (int_ppc_altivec_vabsduw v4i32:$vA, v4i32:$vB))]>;
 
-def : Pat<(v16i8:$vD (abs v16i8:$vA)),
-          (v16i8 (VABSDUB $vA, (v16i8 (V_SET0B))))>;
-def : Pat<(v8i16:$vD (abs v8i16:$vA)),
-          (v8i16 (VABSDUH $vA, (v8i16 (V_SET0H))))>;
-def : Pat<(v4i32:$vD (abs v4i32:$vA)),
-          (v4i32 (VABSDUW $vA, (v4i32 (V_SET0))))>;
-
-def : Pat<(v16i8:$vD (abs (sub v16i8:$vA, v16i8:$vB))),
-          (v16i8 (VABSDUB $vA, $vB))>;
-def : Pat<(v8i16:$vD (abs (sub v8i16:$vA, v8i16:$vB))),
-          (v8i16 (VABSDUH $vA, $vB))>;
-def : Pat<(v4i32:$vD (abs (sub v4i32:$vA, v4i32:$vB))),
-          (v4i32 (VABSDUW $vA, $vB))>;
-
 } // end HasP9Altivec

test/CodeGen/PowerPC/ppc64-P9-vabsd.ll

 ; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
 ; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
 ; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not vabsdu
 
 ; Function Attrs: nounwind readnone
 define <4 x i32> @simple_absv_32(<4 x i32> %a) local_unnamed_addr {
 entry:
   %sub.i = sub <4 x i32> zeroinitializer, %a
   %0 = tail call <4 x i32> @llvm.ppc.altivec.vmaxsw(<4 x i32> %a, <4 x i32> %sub.i)
   ret <4 x i32> %0
 ; CHECK-LABEL: simple_absv_32
-; CHECK: vxor [[ZERO:[0-9]+]], {{[0-9]+}}, {{[0-9]+}}
-; CHECK-NEXT: vabsduw 2, 2, [[ZERO]]
+; CHECK: mtvsrws {{[0-9]+}}, {{[0-9]+}}
+; CHECK-NEXT: vadduwm 2, 2, [[IMM:[0-9]+]]
+; CHECK-NEXT: vabsduw 2, 2, [[IMM]]
 ; CHECK-NEXT: blr
 ; CHECK-PWR8-LABEL: simple_absv_32
 ; CHECK-PWR8: xxlxor
 ; CHECK-PWR8: vsubuwm
 ; CHECK-PWR8: vmaxsw
 ; CHECK-PWR8: blr
 }
 
 ; Function Attrs: nounwind readnone
 define <4 x i32> @simple_absv_32_swap(<4 x i32> %a) local_unnamed_addr {
 entry:
   %sub.i = sub <4 x i32> zeroinitializer, %a
   %0 = tail call <4 x i32> @llvm.ppc.altivec.vmaxsw(<4 x i32> %sub.i, <4 x i32> %a)
   ret <4 x i32> %0
 ; CHECK-LABEL: simple_absv_32_swap
-; CHECK: vxor [[ZERO:[0-9]+]], {{[0-9]+}}, {{[0-9]+}}
-; CHECK-NEXT: vabsduw 2, 2, [[ZERO]]
+; CHECK: mtvsrws {{[0-9]+}}, {{[0-9]+}}
+; CHECK-NEXT: vadduwm 2, 2, [[IMM:[0-9]+]]
+; CHECK-NEXT: vabsduw 2, 2, [[IMM]]
 ; CHECK-NEXT: blr
 ; CHECK-PWR8-LABEL: simple_absv_32_swap
 ; CHECK-PWR8: xxlxor
 ; CHECK-PWR8: vsubuwm
 ; CHECK-PWR8: vmaxsw
 ; CHECK-PWR8: blr
 }
 
 define <8 x i16> @simple_absv_16(<8 x i16> %a) local_unnamed_addr {
 entry:
   %sub.i = sub <8 x i16> zeroinitializer, %a
   %0 = tail call <8 x i16> @llvm.ppc.altivec.vmaxsh(<8 x i16> %a, <8 x i16> %sub.i)
   ret <8 x i16> %0
 ; CHECK-LABEL: simple_absv_16
-; CHECK: vxor [[ZERO:[0-9]+]], {{[0-9]+}}, {{[0-9]+}}
-; CHECK-NEXT: vabsduh 2, 2, [[ZERO]]
+; CHECK: mtvsrws {{[0-9]+}}, {{[0-9]+}}
+; CHECK-NEXT: vadduhm 2, 2, [[IMM:[0-9]+]]
+; CHECK-NEXT: vabsduh 2, 2, [[IMM]]
 ; CHECK-NEXT: blr
 ; CHECK-PWR8-LABEL: simple_absv_16
 ; CHECK-PWR8: xxlxor
 ; CHECK-PWR8: vsubuhm
 ; CHECK-PWR8: vmaxsh
 ; CHECK-PWR8: blr
 }
 
 ; Function Attrs: nounwind readnone
 define <16 x i8> @simple_absv_8(<16 x i8> %a) local_unnamed_addr {
 entry:
   %sub.i = sub <16 x i8> zeroinitializer, %a
   %0 = tail call <16 x i8> @llvm.ppc.altivec.vmaxsb(<16 x i8> %a, <16 x i8> %sub.i)
   ret <16 x i8> %0
 ; CHECK-LABEL: simple_absv_8
-; CHECK: vxor [[ZERO:[0-9]+]], {{[0-9]+}}, {{[0-9]+}}
-; CHECK-NEXT: vabsdub 2, 2, [[ZERO]]
+; CHECK: mtvsrws {{[0-9]+}}, {{[0-9]+}}
+; CHECK-NEXT: vaddubm 2, 2, [[IMM:[0-9]+]]
+; CHECK-NEXT: vabsdub 2, 2, [[IMM]]
 ; CHECK-NEXT: blr
 ; CHECK-PWR8-LABEL: simple_absv_8
 ; CHECK-PWR8: xxlxor
 ; CHECK-PWR8: vsububm
 ; CHECK-PWR8: vmaxsb
 ; CHECK-PWR8: blr
 }
 
 ; The select pattern can only be detected for v4i32.
 ; Function Attrs: norecurse nounwind readnone
 define <4 x i32> @sub_absv_32(<4 x i32> %a, <4 x i32> %b) local_unnamed_addr {
 entry:
   %0 = sub nsw <4 x i32> %a, %b
   %1 = icmp sgt <4 x i32> %0, <i32 -1, i32 -1, i32 -1, i32 -1>
   %2 = sub <4 x i32> zeroinitializer, %0
   %3 = select <4 x i1> %1, <4 x i32> %0, <4 x i32> %2
   ret <4 x i32> %3
 ; CHECK-LABEL: sub_absv_32
-; CHECK: vabsduw 2, 2, 3
+; CHECK: mtvsrws {{[0-9]+}}, {{[0-9]+}}
+; CHECK-DAG: vadduwm 3, 3, {{[0-9]+}}
+; CHECK-DAG: vadduwm 2, 2, {{[0-9]+}}
+; CHECK-NEXT: vabsduw 2, 2, 3
 ; CHECK-NEXT: blr
 ; CHECK-PWR8-LABEL: sub_absv_32
 ; CHECK-PWR8: vsubuwm
 ; CHECK-PWR8: xxlxor
 ; CHECK-PWR8: blr
 }
 
 ; FIXME: This does not produce the ISD::ABS that we are looking for.