[PATCH] D132196: [PowerPC] Add combine logic to use MADDLD/MADDHD/MADDHDU in multiply-add patterns

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17167
+
+  if (MulOp.getOpcode() != ISD::MUL)
+    std::swap(MulOp, AddSubOp);
----------------
I am worried about the swapping for SUB and then adding a SUB(0, swapped_result) to get it back. They are not equal I think.

For example for your below case:
```
define i128 @sub_int128_CmAxB(i64 noundef %a, i64 noundef %b, i64 noundef %c) local_unnamed_addr #0 {
; CHECK-P9-NEXT:    neg 4, 4
; CHECK-P9-NEXT:    maddld 6, 4, 3, 5
; CHECK-P9-NEXT:    maddhd 4, 4, 3, 5
; CHECK-P9-NEXT:    mr 3, 6
; CHECK-P9-NEXT:    blr
entry:
  %conv = sext i64 %c to i128
  %conv1 = sext i64 %a to i128
  %conv2 = sext i64 %b to i128
  %mul = mul nsw i128 %conv2, %conv1
  %sub = sub nsw i128 %conv, %mul
  ret i128 %sub
}
```

Suppose `%b` is the most negative value.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17179
+  // should have zero sign bit which map to unsigned, all others depend on
+  // SignBitIsZero.
+  auto IsLegalOperand = [&DAG](SDValue Op, unsigned &NumSignBits,
----------------
`maddld` and `maddhd` are valid for 64 bit integers which include sign bit at the bit-0. So I think here we should expect there are at least 64 + 1= 65 sign bits?
For the 64 sign bits and zero sign bit case, I guess the case is like: we have 0 in all the high 64 bits, and we have 1 in the first bit of the low 64 bits?(Otherwise, the sign bits number must be at least 65?) If so, this is not a case can be handled correctly either. We are expecting zero extension, but the low 64 bits value which can be accessed by maddld/maddhd is a signed value.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17217
+      IsUnsigned = OpIsUnsigned;
+    else if (OpIsUnsigned != IsUnsigned)
+      return SDValue();
----------------
This seems overkill. For signed values, cases that some operands are negative while others are positive are still valid.

I think for `add(mul(A, B), C)`,
- if A, B C all have zero sign bit, it is equal to maddhdu + maddld.
- if any of A, B, C can not be proven have zero sign bit, it is equal to maddhd + maddld



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132196