[PATCH] D90113: [DAGCombiner] Fold BinOp into Select containing identity constant

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:536
     SDValue foldVSelectOfConstants(SDNode *N);
-    SDValue foldBinOpIntoSelect(SDNode *BO);
+    SDValue foldBinOpIntoSelect(SDNode *BO, APInt *IdentityInt = nullptr,
+                                bool RHSIdentityOnly = false);
----------------
Maybe pass IdentityInt in as an Optional<int64_t> to simplify all the callers?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2097
 
-  // Don't do this unless the old select is going away. We want to eliminate the
-  // binary operator, not replace a binop with a select.
-  // TODO: Handle ISD::SELECT_CC.
+  // Don't do this unless the select has more than one use.
+  // TODO: Handle ISD::SELECT_CC and ISD:VSELECT.
----------------
"Don't do this **if** the select has more than one use", surely?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2126
+
+    for (char Swap = 0; !CanFoldInAddr && Swap <= 1; Swap++) {
+      ConstantSDNode *IdentOp = isConstOrConstSplat(Swap ? CF : CT);
----------------
Use int or unsigned instead of char.


================
Comment at: llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-idiv.ll:339-341
+; GCN-NEXT:    v_add_i32_e32 v2, vcc, 1, v3
 ; GCN-NEXT:    v_cmp_ge_f32_e64 vcc, |v1|, v0
+; GCN-NEXT:    v_cndmask_b32_e32 v0, v3, v2, vcc
----------------
Regression here and in lots of other tests below. It looks like it's no longer using an add-with-carry instruction to implement x+(cond?1:0).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90113