[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/Analysis/ValueTracking.cpp:4889
+    if (match(I, m_Select(m_Specific(ValAssumedPoison), m_Value(), m_Value())))
+      return true;
   }
----------------
aqjune wrote:
> This update was necessary to add transformations for pattern
> `merge_two_logical_ands3`: (X && Y) && X -> X && Y
> `merge_two_logical_ors3`: (X || Y) || X -> X || Y
> 
> The remaining patterns are supported by the updates in InstCombineSelect.cpp.
Just as a side-note, I think the right way to handle this is to replace `propagatesPoison()` with `getPoisonPropagatingOperands()` or similar, which allows handling cases like these, where poison is only partially propagated. For now this is fine, though I think it would be better to write it as:

```
if (const auto *SI = dyn_cast<SelectInst>(I))
  return directlyImpliesPoison(ValAssumedPoison, SI->getCondition(), Depth + 1);
```

This will keep the same recursion as for the propagatesPoison() case.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2632
+
+    // select a, (a /\ b), false -> select a, b, false
+    Value *A, *B;
----------------
Please use `&&` and `||` instead of `/\` and `\/`.


================
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);
+
----------------
What currently happens for something like `a ? (a | b) : false`? I assume it doesn't get folded?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96945