[PATCH] D153963: [InstCombine] Fold binop of select and cast of select condition

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:898
+                               m_Value(FalseVal))))
+    CastOp = RHS;
+  else
----------------
Can you put this duplicate logic for RHS/LHS in a lambda?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:903
+  auto NewFoldedConst = [&](bool IsTrueArm, bool IsCastOpRHS, Value *V,
+                            bool IsZExt = false) {
+    if (IsTrueArm) {
----------------
Since you capture the entire scope I think the `IsCastOpRHS` and `IsZExt` can just be computed in the lambda which makes the callsites clearer.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:917
+      return Builder.CreateBinOp(Opc, V, CastConst);
+    return Builder.CreateBinOp(Opc, CastConst, V);
+  };
----------------
Imo cleaner as a whole is to seperate the decision about the constant from creating the binop i.e

```
Constant * C;
if(IsTrueArm) {
  C = 0
}
else if(IsZext) {
  C = 1
}
else {
  C = AllOnes
}

return IsConstOpRHS ? BinOp(Opc, V, C) : BinOp(Opc, C, V);
```
```


================
Comment at: llvm/test/Transforms/InstCombine/binop-select-cast-of-select-cond.ll:197
+  ret <2 x i64> %add
+}
----------------
goldstein.w.n wrote:
> antoniofrighetto wrote:
> > goldstein.w.n wrote:
> > > Can you split the tests to a seperate commit so we can see the diff generated by this patch?
> > Do you mean to open a different patch for the tests (or land them directly)?
> Open a different patch.
still outstanding.


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

https://reviews.llvm.org/D153963