[RISCV] Fold binary op into select if profitable.

[mgudim]

Summary

Consider the following pattern binOp (select cond, x, c0), c1.
Where c0 and c1 are constants.
We can transform it to select cond, binOp(x, c1), binOp(c0, c1).

If binOp(c0, c1) ends up being 0 or -1 we can turn the select into
a more profitable sequence.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5527

dyn_cast->cast if it can't fail

5527

const APInt &

[mgudim]

Addressed comments.
Fixed a bug: the constants were passed to DAG.FoldConstantArithmetic in the wrong order.
Moved call to foldBinOpIntoSelectIfProfitable after combineSelectToBinOp - this way we will not fold binop if the constant of select is already 0 or -1.
Minor typo fixes.

[mgudim]

Pre-merge checks passed.

[craig.topper]

LGTM

[mgudim]

Thanks @craig.topper , I'll merge it (once I figure out the process). What about https://reviews.llvm.org/D151750 Can it be merged after this one?

[jrtc27]

This was committed as d0189584631e587279ee5f0af5feb94d8045bb31 without the normal Phab metadata (i.e. without arc patch), but is causing assertion failures building the Linux kernel per https://github.com/llvm/llvm-project/issues/63293.

Please revert this, debug the issue and re-land with an appropriate fix and the correct metadata per the LLVM Phabricator documentation (https://llvm.org/docs/Phabricator.html#committing-a-change).

[craig.topper]

Please revert. I think the issue is that it never checks the opcode of the binary operator which might not even be a binary operator. It might not even be something that FoldConstantArithmetic can handle.

[craig.topper]

I have reverted the change f9d0bf06319203a8cbb47d89c2f39d2c782f3887

[mgudim]

added logic for the correct order of operands when creating new opepands of select.
added test for all four possible positions of constants in select and binop.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5700

Please upload patches with full context using git diff -U99999

[mgudim]

Uploaded diff with -u99999 for more context.

[mgudim]

Can't reproduce the failed tests locally. Put a print statement in the code to make sure it is the most recent patch being tested.

[mgudim]

(1) Added a check if FoldConstantArithmetic returns null.
(2) Fixed the incorrect use of ArrayRef. This was causing a bug, which is not visible when compiled in Debug mode.

[mgudim]

Changed ArrayRef to SmallVector in two more lines, which I missed in the previous update.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5818

Use a plain old C array?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5803

You can use a plain C array and std::swap(NewConstOps[0], NewConstOps[1]) if SelOpNo == 1.

[mgudim]

Addressed comments: use plain array instead of SmallVector

[mgudim]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5803

Makes sence. Done.

[mgudim]

@craig.topper is it ok to recommit this?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5784

Can ConstSelOp and ConstSelOpNode be moved below the if by using ConstSelOpNo?

5846

Is this safe for division/remainder? The select could be sanitizing inputs. RISC-V doesn't have traps for div/rem, but IR/SelectionDAG semantics consider division by 0 to be immediate UB. Same with (sdiv -1, INT_MIN)

[mgudim]

addressed comments:
(1) Moved definitions of ConstSelOp and ConstSelOpNode lower.
(2) Added check DAG.isSafeToSpeculativelyExecute(...) to check if commuting select and binop is legal.

[mgudim]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5846

Thanks, I never thought about that.

[craig.topper]

LGTM

[mgudim]

rebased on current main.

[mgudim]

I have accidentally commited this in "Needs Review" state. What should I do?

[craig.topper]

In D152147#4502074, @mgudim wrote:

I have accidentally commited this in "Needs Review" state. What should I do?

It was only in Needs Review due to @jrtc27's. I believe those comments were addressed. The new commit has the phabricator metadata. I think you're ok unless Jessica speaks up.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

   return SDValue();
 }
 
+// Transform `binOp (select cond, x, c0), c1` where `c0` and `c1` are constants
+// into `select cond, binOp(x, c1), binOp(c0, c1)` if profitable.
+// For now we only consider transformation profitable if `binOp(c0, c1)` ends up
+// being `0` or `-1`. In such cases we can replace `select` with `and`.
+// TODO: Should we also do this if `binOp(c0, c1)` is cheaper to materialize
+// than `c0`?
+static SDValue
+foldBinOpIntoSelectIfProfitable(SDNode *BO, SelectionDAG &DAG,
+                                const RISCVSubtarget &Subtarget) {
+  if (Subtarget.hasShortForwardBranchOpt())
+    return SDValue();
+
+  unsigned SelOpNo = 0;
+  SDValue Sel = BO->getOperand(0);
+  if (Sel.getOpcode() != ISD::SELECT || !Sel.hasOneUse()) {
+    SelOpNo = 1;
+    Sel = BO->getOperand(1);
+  }
+
+  if (Sel.getOpcode() != ISD::SELECT || !Sel.hasOneUse())
+    return SDValue();
+
+  unsigned ConstSelOpNo = 1;
+  unsigned OtherSelOpNo = 2;
+  SDValue ConstSelOp = Sel->getOperand(1);
+  ConstantSDNode *ConstSelOpNode = dyn_cast<ConstantSDNode>(ConstSelOp);
+  if (!ConstSelOpNode) {
+    ConstSelOpNo = 2;
+    OtherSelOpNo = 1;
+    ConstSelOp = Sel->getOperand(2);
+    ConstSelOpNode = dyn_cast<ConstantSDNode>(ConstSelOp);
+  }
+  if (!ConstSelOpNode)
+    return SDValue();
+  if (!ConstSelOpNode || ConstSelOpNode->isOpaque())
+    return SDValue();
+
+  SDValue ConstBinOp = BO->getOperand(SelOpNo ^ 1);
+  ConstantSDNode *ConstBinOpNode = dyn_cast<ConstantSDNode>(ConstBinOp);
+  if (!ConstBinOpNode || ConstBinOpNode->isOpaque())
+    return SDValue();
+
+  SDLoc DL(Sel);
+  EVT VT = BO->getValueType(0);
+
+  SDValue NewConstOp = DAG.FoldConstantArithmetic(BO->getOpcode(), DL, VT,
+                                                  {ConstSelOp, ConstBinOp});
+  const APInt &NewConstAPInt = cast<ConstantSDNode>(NewConstOp)->getAPIntValue();
+  if (!NewConstAPInt.isZero() && !NewConstAPInt.isAllOnes())
+    return SDValue();
+
+  SDValue NewNonConstOp = DAG.getNode(
+      BO->getOpcode(), DL, VT, Sel->getOperand(OtherSelOpNo), ConstBinOp);
+  SDValue NewT = (ConstSelOpNo == 1) ? NewConstOp : NewNonConstOp;
+  SDValue NewF = (ConstSelOpNo == 1) ? NewNonConstOp : NewConstOp;
+  return DAG.getSelect(DL, VT, Sel.getOperand(0), NewT, NewF);
+}
+
 SDValue RISCVTargetLowering::lowerSELECT(SDValue Op, SelectionDAG &DAG) const {
   SDValue CondV = Op.getOperand(0);
   SDValue TrueV = Op.getOperand(1);

   if (SDValue V = combineSelectToBinOp(Op.getNode(), DAG, Subtarget))
     return V;
 
+  if (Op.hasOneUse() && isBinOp(Op->use_begin()->getOpcode())) {
+    SDNode *BinOp = *Op->use_begin();
+    if (SDValue NewSel =
+            foldBinOpIntoSelectIfProfitable(*Op->use_begin(), DAG, Subtarget)) {
+      DAG.ReplaceAllUsesWith(BinOp, &NewSel);
+      return lowerSELECT(NewSel, DAG);
+    }
+  }
+
+
   // If the condition is not an integer SETCC which operates on XLenVT, we need
   // to emit a RISCVISD::SELECT_CC comparing the condition to zero. i.e.:
   // (select condv, truev, falsev)

llvm/test/CodeGen/RISCV/fold-binop-into-select.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mtriple=riscv64 < %s  | FileCheck  %s
+
+define i64 @select_add(i1 %c, i64 %x) {
+; CHECK-LABEL: select_add:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a0, a0, 1
+; CHECK-NEXT:    addi a1, a1, 2
+; CHECK-NEXT:    addi a0, a0, -1
+; CHECK-NEXT:    and a0, a0, a1
+; CHECK-NEXT:    ret
+entry:
+  %select_ = select i1 %c, i64 -2, i64 %x
+  %res = add i64 %select_, 2
+  ret i64 %res
+}
+
+define i64 @select_and(i1 %c, i64 %x) {
+; CHECK-LABEL: select_and:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a0, a0, 1
+; CHECK-NEXT:    addiw a0, a0, -1
+; CHECK-NEXT:    and a0, a1, a0
+; CHECK-NEXT:    andi a0, a0, 64
+; CHECK-NEXT:    ret
+entry:
+  %select_ = select i1 %c, i64 63, i64 %x
+  %res = and i64 %select_, 64
+  ret i64 %res
+}
+