[PATCH] D134277: [RISCV] Combine comparison and logic ops.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8305
+    assert(Pos < Size && "Out of range\n");
+    return Ops.at(Pos);
+  }
----------------
Why `at` instead of `[Pos]`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8311
+  // correspondence info about comparison operations.
+  static llvm::Optional<std::pair<CmpOpInfo, CmpOpInfo>>
+  getInfoAbout(SDValue const &CmpOp0, SDValue const &CmpOp1) {
----------------
std::optional


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8316
+    if (!establishCorrespondence(Op0, Op1))
+      return None;
+    return std::make_pair(Op0, Op1);
----------------
std::nullopt


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8348
+//     e.g. C > A will be swapped to A < C.
+static Optional<std::tuple<ISD::CondCode, SDValue, SDValue, SDValue>>
+verifyCompareConds(SDNode *N, SelectionDAG &DAG) {
----------------
std::optional


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8360
+  if (!V0.hasOneUse() || !V1.hasOneUse())
+    return None;
+
----------------
std::nullopt


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8363
+  // Condition 2. Operands have to be comparison operations.
+  if (V0->getOpcode() != ISD::SETCC || V1->getOpcode() != ISD::SETCC)
+    return None;
----------------
V0 and V1 are SDValue. We should use `.getOpcode()` instead of `->getOpcode()`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8367
+  // Condition 3.1. Operations only with integers.
+  if (!V0->getOperand(0).getValueType().isInteger())
+    return None;
----------------
`.getOperand(0)`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8425
+  const unsigned TyIdx = static_cast<unsigned>(OpT);
+  assert(TyIdx < 2 && "Access out of boundaries");
+  return SelectionCodes[TyIdx][ChooseSelCode];
----------------
I'm not sure the enum makes sense if you have to assert that it isn't once of the values. Can't we just have a `bool IsUnsigned`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8437
+
+  const auto BitOpcode = N->getOpcode();
+  assert((BitOpcode == ISD::AND || BitOpcode == ISD::OR) &&
----------------
LLVM is pretty conservative about the use of `auto`. Use `unsigned` here.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8447
+  const auto [RefOpcode, A, B, C] = Props.value();
+  const auto CmpOpVT = A.getValueType();
+
----------------
use EVT instead of auto.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8451
+  OperationType TypeOfCmp = OperationType::None;
+  if (ISD::isUnsignedIntSetCC(RefOpcode))
+    TypeOfCmp = OperationType::Unsigned;
----------------
Seems like rather than an enum we can have

```
bool IsUnsigned = ISD::isUnsignedIntSetCC(RefOpcode);
assert((IsUnsigned || ISD::isSignedIntSetCC(RefOpcode)) &&
       "Operation neither with signed or unsigned integers.");
``


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134277