[PATCH] D122563: [RISCV] Add DAGCombine to fold base operation and reduction.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7244
+  unsigned Opc = N->getOpcode();
+  int reduceIdx;
+  if (isReduction(N->getOperand(0), Opc))
----------------
Why `int` instead of `unsigned`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7245
+  int reduceIdx;
+  if (isReduction(N->getOperand(0), Opc))
+    reduceIdx = 0;
----------------
Capitalize


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7252
+
+  // Skip if FADD disallow reassocation but the combiner needs.
+  if (Opc == ISD::FADD && !N->getFlags().hasAllowReassociation() &&
----------------
reassocation->reassociation


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7257
+
+  auto *Extract = N->getOperand(reduceIdx).getNode();
+  auto *Reduce = Extract->getOperand(0).getNode();
----------------
Why getNode()?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7271
+  // TODO: Deal with value other than neutral element.
+  auto isRVVNeutralElement = [&Opc, &DAG](SDNode *N, SDValue V) {
+    if (Opc == ISD::FADD && N->getFlags().hasNoSignedZeros()) {
----------------
Pass `Opc` by value not by reference


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7273
+    if (Opc == ISD::FADD && N->getFlags().hasNoSignedZeros()) {
+      auto C = dyn_cast<ConstantFPSDNode>(V.getNode());
+      return C && C->isZero();
----------------
`auto C` -> `auto *C` We always want to be able to see if something is a pointer.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7280
+
+  // Check the scalar of ScalarV is netrual element
+  if (!isRVVNeutralElement(N, ScalarV.getOperand(1)))
----------------
netrual -> neutral


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7289
+  EVT VT = ScalarV.getValueType();
+  int anotherOpIdx = !reduceIdx;
+  SDValue NewStart = N->getOperand(anotherOpIdx);
----------------
Capitalize `anotherOpIdx`

Use (1 - ReduceIdx) instead of !reduceIdx.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7293
+  if (VT.isInteger()) {
+    auto C = dyn_cast<ConstantSDNode>(NewStart.getNode());
+    if (!C || C->isZero() || !isInt<5>(C->getSExtValue()))
----------------
`auto C` -> `auto *C`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7300
+
+  SDValue NewScalarV = DAG.getNode(SplatOpc, DL, VT, ScalarV->getOperand(0),
+                                   NewStart, ScalarV->getOperand(2));
----------------
Use `ScalarV.getOperand(0)` instead of `ScalarV->getOperand(0)`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7302
+                                   NewStart, ScalarV->getOperand(2));
+  auto NewReduce = DAG.UpdateNodeOperands(
+      Reduce, Reduce->getOperand(0), Reduce->getOperand(1), NewScalarV,
----------------
Don't use UpdateNodeOperands it scares me because it can trigger surprising DAG changes that can invalidate the SDValues you're holding.. Use getNode to create new nodes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122563