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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

IMO, this version is better than the first one, after the type legalization recognizing the legalized i128 types seems more complicated than recognizing the MADD pattern for type i128 before the type legalization.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17165
+  SDValue MulOp = N->getOperand(0).getOpcode() == ISD::MUL ? N->getOperand(0)
+                                                           : N->getOperand(1);
+  // Do not transform if there are other users of the mul.
----------------
Can we do a std::swap first to put the MUL operand to a settled index? So that we don't need to check MUL operand many times in below code


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17184
+  bool MulLSignBitIsZero = DAG.SignBitIsZero(MulLHS);
+  if (MulLSignBits == 64 && !MulLSignBitIsZero)
+    return SDValue();
----------------
We may need some false check test cases here, like extending i65 to i128?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17249
+                                   Op0, Op1, Op2, DAG, dl);
+  SDValue Combined = DAG.getNode(ISD::BUILD_PAIR, dl, MVT::i128, MAddL, MAddH);
+  return Combined;
----------------
Maybe we need to explicitly check this function is called before type legalization(`isBeforeLegalize`)? `BUILD_PAIR` is a node which should only be generated before type legalization.



================
Comment at: llvm/test/CodeGen/PowerPC/add-sub-int128-madd.ll:4
 
 define i128 @add_int128(i64 noundef %a, i64 noundef %b, i64 noundef %c) local_unnamed_addr #0 {
 ; CHECK-P9-LABEL: add_int128:
----------------
Can we add some cases for other types extension like i32/i16 to i128? I think for these cases, we may don't need maddhd/maddhdu to generate the higher 64 bits of the i128.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132196