[PATCH] D58950: [PowerPC] Strength reduction of multiply by a constant by shift and add/sub in place

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Comments mostly related to comments and testcases.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14584
+      return false;
+    // TODO: enhance the condion for subtarget before pwr8
+    case PPC::DIR_PWR8:
----------------
condion => condition, and move to before 'return'?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14588
+      // Only do (mul x, -(2^N + 1)) => -(add (shl x, N), x) for vector type
+      // because vector mul has more cycles.
+      return IsAddOne && IsNeg ? VT.isVector() : true;
----------------
This comments is confusing: all four cases are using vector mul, so "because vector mul has more cycles" can not explain why we want to do the case of `IsAddone && IsNeg` for vector type only.

Can we add comments about the cycles before and after combine, maybe a table so that it will clear about why the combine is profitable for specific type/pattern here ?
eg:
```
  type         mul     add  shl   add+shl .  sub+add+shl      sub+shl
 scalar         ?         ?       ? 
vector          ?          ?     ?
```



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14601
+  // (mul x, 2^N + 1) => (add (shl x, N), x)
+  // (mul x, -(2^N + 1)) => -(add (shl x, N), x)
+  if ((MulAmtAbs - 1).isPowerOf2()) {
----------------
Maybe we should move these comments after `if` similar to `else if` below?


================
Comment at: llvm/test/CodeGen/PowerPC/mul-const-vector.ll:9
+; CHECK-LABEL: test1_v16i8:
+; CHECK: vspltisb v[[REG1:[0-9]+]], 4
+; CHECK-NOT: vmul
----------------
Looks like `vsplitsb`/`xxsplitb` is the only difference for P8/P9, you might be better using common prefixes to share (and reduce) checks.

```
... -mcpu=pwr9 ... --check-prefixes=CHECK,CHECK-P9
... -mcpu=pwr8 --check-prefixes=CHECK,CHECK-P8

; CHECK-LABEL: test1_v16i8:
; CHECK-P8:vspltisb v[[REG1:[0-9]+]], 4
; CHECK-P9: xxspltib v[[REG1:[0-9]+]], 4
; CHECK-NOT: vmul
; CHECK-NEXT: vslb v[[REG2:[0-9]+]], v2, v[[REG1]]

```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58950