[PATCH] D153502: [DAGCombiner] Change foldAndOrOfSETCC() to optimize and/or patterns

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6036
+  // predicate and have a common operand.
+  auto AreSameCMPsAndHaveCommonOperand = [&](SDValue Cmp0, SDValue Cmp1) {
+    if (Cmp0->getOpcode() != ISD::SETCC || Cmp1->getOpcode() != ISD::SETCC)
----------------
It is weird to pass in LHS and RHS as arguments to this function, but then examine their operands LHS0 etc by capture from the enclosing scope. I suggest changing this function to take no arguments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6039
+      return false;
+    // Check if the compare instrucions have only one use.
+    if (!Cmp0->hasOneUse() || !Cmp1->hasOneUse())
----------------
Don't add comments that just say *what* the code does. If there is nothing interesting to say about *why* you are checking for one use, just remove the comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6067
+  //
+  // LHS0 LHS1        RHS0 RHS1        LHS0 RHS0
+  //    \ /              \ /              \ /
----------------
I would suggest writing these on a single line, like most of the other comments in this file, e.g.:
```
(lhs0 < lhs1) | (rhs0 < rhs2) -> min(lhs0, rhs0) < lhs1
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6081
+      TLI.isOperationLegal(ISD::SMAX, OpVT) &&
+      TargetPreference == AndOrSETCCFoldKind::None &&
+      AreSameCMPsAndHaveCommonOperand(LHS, RHS)) {
----------------
It seems wrong to check TargetPreference here. TargetPreference currently only controls the desired combine behaviour for a combination of eq or ne comparisons. Your new combine does not overlap with this, since it only works for non-eq/ne comparisons. So why would you only do it if the target does *not* want to combine eq/ne comparisons?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:6087
+    ISD::CondCode CC = CCL;
+    if (LHS0 == RHS0) {
+      CommonValue = LHS0;
----------------
AreSameCMPsAndHaveCommonOperand already checks the various different cases of CCL==CCR etc and LHS0==RHS0 etc, and now you have to check them again here. To avoid the duplication why not just inline AreSameCMPsAndHaveCommonOperand here? You could set `CC=SETCC_INVALID` at the start, then go through the cases, then test `CC==SETCC_INVALID` to see if any of them matched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153502