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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

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).



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:330
     void transferMemOperands(SDNode *N, SDNode *Result);
+    MachineSDNode *flipSignBit(const SDValue &N, SelectionDAG *DAG,
+                               const SDLoc &dl, SDNode **SignBitVec = nullptr);
----------------
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`.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3977
+/// of vector integer type. Additionally, if SignBitVec is non-null,
+/// this method returns a node with one at MSB of all elements 
+/// and zero at other bits in SignBitVec.
----------------
Since you're setting the output parameter rather than returning it...
s/returns/sets it to


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3992
+  }
+  else if (VecVT == MVT::v8i16) {
+    SDNode *Hi = DAG->getMachineNode(PPC::LIS, dl, MVT::i32,
----------------
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.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4842
+    bool SkipAdjust = false;
+    if (N->getOperand(0).getOpcode() == ISD::SUB &&
+        N->getOperand(0)->getOperand(0).getOpcode() == ISD::ZERO_EXTEND &&
----------------
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)?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4856
+
+    if (SkipAdjust)
+      AbsOp = CurDAG->getMachineNode(AbsOpcode, dl, VecVT,
----------------
Can you please return here so the remaining code does not need to be nested into an `else`?


https://reviews.llvm.org/D45522