[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:8315
+  // Condition 1. Operations have to have same value type.
+  if (V0.getValueType() != V1.getValueType()) {
+    return None;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8319
+  // Condition 2.1. Operations have to be used only in logic operation.
+  if (!V0.hasOneUse() || !V1.hasOneUse()) {
+    return None;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8323
+
+  SDNode *N0 = V0.getNode();
+  SDNode *N1 = V1.getNode();
----------------
Why do we need to get the underlying SDNode* explicitly? SDValue has a `operator->`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8358
+                               std::distance(N1->op_begin(), CommonOpIt1));
+  auto CheckZeroOne = [](std::pair<int, int> P) {
+    return (P.first == 0 || P.first == 1) && (P.second == 0 || P.second == 1);
----------------
Does this get reported as unused in release builds?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8419
+                            const RISCVSubtarget &Subtarget) {
+  if (!Subtarget.hasStdExtZbb()) {
+    return SDValue();
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8435
+  // Collection of all Conditions which similar to less.
+  static const std::unordered_set<ISD::CondCode> SetLess = {
+      ISD::SETULT, ISD::SETULE, ISD::SETLT, ISD::SETLE};
----------------
All of the less conditions have bit 1 set and bit 2 clear. All the greater have bit 1 clear and bit 2 set. That's by design. Can we use that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134277