[PATCH] D107603: Fold (add (select lhs, rhs, cc, 0, y), x) -> (select lhs, rhs, cc, x, (add x, y))

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

A general observation is that expanding this to `ISD::SELECT` now opens this up to vector types. I think the only thing that's now preventing it is `isZeroOrAllOnes` which "silently" (for want of a better word) only accepts scalar constants.

My first thoughts are that we could either be clearer about not accepting vector types, or - without wanting to expand the scope of this patch - we do open it up to vectors. From a quick play around on top of your patch it doesn't look too useful in that case, though. A vector `add_select_all_zeros_v4i32` just introduces an extra `vsetvli`.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5775
   FalseVal = DAG.getNode(N->getOpcode(), SDLoc(N), VT, OtherOp, NonConstantVal);
-  // Unless SwapSelectOps says CC should be false.
+  // Unless SwapSelectOps says condition should be false.
   if (SwapSelectOps)
----------------
"the" condition, perhaps


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5803
+  //      (select lhs, rhs, cc, x, (add x, y))
+  return combineSelectAndUseCommutative(N, DAG, false);
+}
----------------
I know you didn't introduce this but `/*AllOnes*/` might be a good change to make while you're here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107603