[PATCH] D96945: [InstCombine] Add simplification of two logical and/ors

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2640
+        match(FalseVal, m_c_LogicalOr(m_Specific(CondVal), m_Value(B))))
+      return replaceOperand(SI, 2, B);
+
----------------
nikic wrote:
> aqjune wrote:
> > aqjune wrote:
> > > nikic wrote:
> > > > What currently happens for something like `a ? (a | b) : false`? I assume it doesn't get folded?
> > > No, it doesn't get folded.
> > > That being said, I think it is better to simply propagate true/false to select operands, e.g.
> > > `a ? (a | b) : false` -> `a ? (true | b) : false` -> `a ? b : false`. I'll update this patch to do so.
> > Nevermind, since the replacement needs to take care of # uses but the current version doesn't need it, I think it does not completely cover the foldings in this patch.
> What I had in mind here is that this is basically the `foldSelectValueEquivalence()` fold, just that instead of `x == C ? y : z` we have `x == true ? y : z`, with the `== true` comparison being implicit. If we generalize that fold to handle this special case, this should work automatically (either via SimplifyWithOpReplaced, or by direct operand replacement for single use).
Ignoring the case of actual operand replacement, I think doing something like this should do:

```
if (Value *S = SimplifyWithOpReplaced(TrueVal, CondVal, True, SQ, /* AllowRefinement */ true))
  return replaceOperand(SI, 1, S);
if (Value *S = SimplifyWithOpReplaced(FalseVal, CondVal, False, SQ, /* AllowRefinement */ true))
  return replaceOperand(SI, 2, S);
```

Though probably SimplifyWithOpReplaced needs to be strengthened a bit to also handle selects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96945