[PATCH] D54783: [Power9] suboptimal vec_abs for some cases

[Kewen Lin via Phabricator via llvm-commits]

jedilyn marked 12 inline comments as done.
jedilyn added a comment.

Hi Jinsong @jsji,  thanks for your time! I'll break down it to some separate patches and add more comments later.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:657
 
+    if (!Subtarget.hasP8Altivec())
+      setOperationAction(ISD::ABS, MVT::v2i64, Expand);
----------------
jsji wrote:
> Can we add some comment here for why we need to handle v2i64 differently? due to current subtarget doesn't support smax v2i64?
Yes, I'll add more comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9572
+
+  // SMAX patch hasn't landed yet, so use intrinsic first
+  // TODO: Should use SMAX directly once SMAX patch landed
----------------
jsji wrote:
> Which SMAX patch? can you be specific? Has it landed now?
The one is D47332, it isn't landed yet. I'll add more comments here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:13267
+  case ISD::VSELECT: 
+    return combineVSelect(N, DCI);
   }
----------------
jsji wrote:
> Although combineVSelect is also for VABSD, this looks like another independent optimization, can we split this into another patch?
Sure. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14526
+
+SDValue PPCTargetLowering::combineABS(SDNode *N, DAGCombinerInfo &DCI) const {
+  assert((N->getOpcode() == ISD::ABS) && "Need ABS node here");
----------------
jsji wrote:
> Can we add some comments before this function, to show what it will transform from & to?
Sure.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14562
+
+SDValue PPCTargetLowering::combineVSelect(SDNode *N,
+                                          DAGCombinerInfo &DCI) const {
----------------
jsji wrote:
> Can we add some comments before this function, to show what it will transform from & to?
Sure.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:381
+      /// operand #2 constant i32 0 or 1, to indicate whether needs to patch
+      /// the most significant bit for signed i32
+      ABSD,
----------------
jsji wrote:
> Can we have some example code to show why we need to patch the bit, similar to old code for ISD::ABS
> 
> ```   
> // For abs(sub(a, b)), we generate VABSDUW(a+0x80000000, b+0x80000000).
> ...
> ```
Good idea, I'll add more comments here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:382
+      /// the most significant bit for signed i32
+      ABSD,
+
----------------
jsji wrote:
> Since this node will only handle vector types, maybe we should rename it to `VABSD`?
OK, I will use `VABSD`instead.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:4048
+
+  def : Pat<(v4i32 (PPCabsd v4i32:$A, v4i32:$B, (i32 1))),
+            (v4i32 (VABSDUW (XVNEGSP $A), (XVNEGSP $B)))>;
----------------
jsji wrote:
> Can we add comment here for the last operand
Since we have added more comments to describe the last operand usage for `ABSD` in target specific `ISDNODE` header file, does it make sense just leaving this here? Although to supplement more comments here is pretty easy, it probably brings more efforts to maintain the description and keep them always sync in different places.


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-P9-vabsd.ll:1
-; 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
+; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names -verify-machineinstrs | FileCheck %s
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names -verify-machineinstrs | FileCheck %s
----------------
jsji wrote:
> Can we add -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names  in a NFC patch before this?
Good idea. I'll split it.


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-P9-vabsd.ll:103
+
+define <8 x i16> @sub_absv_16(<8 x i16> %a, <8 x i16> %b) local_unnamed_addr {
+entry:
----------------
jsji wrote:
> This can be in the new patch for combineVSelect.
Yes, thanks!


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-P9-vabsd.ll:470
 
+; To verify vabsdu* exploitation for ucmp + sub + select sequence
+
----------------
jsji wrote:
> This can be in new combineVselect patch as well.
OK


================
Comment at: llvm/test/CodeGen/PowerPC/pre-inc-disable.ll:22
 ; CHECK:    xxpermdi v5, f0, f0, 2
-; CHECK:    vperm v0, v4, v5, v2
-; CHECK:    vperm v5, v5, v4, v3
-; CHECK:    xvnegsp v5, v5
-; CHECK:    xvnegsp v0, v0
+; CHECK-DAG: vperm v[[VR1:[0-9]+]], v4, v5, v2
+; CHECK-DAG: vperm v[[VR2:[0-9]+]], v5, v4, v3
----------------
jsji wrote:
> are these `CHECK` -> `CHECK-DAG` relevant to this change? Or just non-relevant NFC change?If so , please commit before this patch. Thanks.
Good point, thanks. It can be a NFC patch to make the case more flexible. Technically speaking, the original case isn't very robust, the order and assigned register may change.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54783/new/

https://reviews.llvm.org/D54783