[llvm] [ValueTracking][X86] Compute KnownBits for phadd/phsub (PR #92429)

[via llvm-commits]

================
@@ -950,6 +950,39 @@ getKnownBitsFromAndXorOr(const Operator *I, const APInt &DemandedElts,
   return KnownOut;
 }
 
+static KnownBits computeKnownBitsForHorizontalOperation(
+    const Operator *I, const APInt &DemandedElts, unsigned Depth,
+    const SimplifyQuery &Q,
+    const function_ref<KnownBits(const KnownBits &, const KnownBits &)>
+        KnownBitsFunc) {
+  APInt DemandedEltsLHS, DemandedEltsRHS;
+  getHorizDemandedEltsForFirstOperand(Q.DL.getTypeSizeInBits(I->getType()),
+                                      DemandedElts, DemandedEltsLHS,
+                                      DemandedEltsRHS);
+
+  const auto ComputeForSingleOpFunc =
+      [Depth, &Q](const Value *Op, APInt &DemandedEltsOp,
+                  std::array<KnownBits, 2> &Known) {
+        for (unsigned Index = 0; Index < Known.size(); ++Index) {
+          if (!DemandedEltsOp.isZero()) {
+            Known[Index] = computeKnownBits(Op, DemandedEltsOp, Depth + 1, Q);
+          } else {
+            Known[Index] = KnownBits(getBitWidth(Op->getType(), Q.DL));
+            Known[Index].setAllZero();
----------------
mskamp wrote:

When looking at the code again, I'd agree that it is not at all clear.

“skipped” means that the `else` case applies, right? If this is the case, then it's in fact either all or nothing: Either the condition is true for both values of `Index` or it is false for both values (under the assumption that we don't have vectors with an odd number of elements).

The latter case happens for example in the test `hadd_extract_2st_trunc_v4i32`. Here, we have 4 vector elements but are only interested in the 2nd. `phadd` uses only its first operand to compute the 2nd vector element. Therefore, `DemandedEltsRHS = 0`.

To improve the readability of the function, I'd suggest to completely get rid of the `if` statement in the lambda and use a case distinction in the top-level function. After applying the other suggestion, the function would look like this:
```cpp
  const auto ComputeForSingleOpFunc =
      [Depth, &Q, KnownBitsFunc](const Value *Op, APInt &DemandedEltsOp) {
        std::array<KnownBits, 2> Known;
        for (unsigned Index = 0; Index < Known.size(); ++Index) {
          Known[Index] = computeKnownBits(Op, DemandedEltsOp, Depth + 1, Q);
          DemandedEltsOp <<= 1;
        }
        return KnownBitsFunc(Known[0], Known[1]);
      };

  if (!DemandedEltsLHS.isZero() && !DemandedEltsRHS.isZero()) {
    return ComputeForSingleOpFunc(I->getOperand(0), DemandedEltsLHS)
        .intersectWith(ComputeForSingleOpFunc(I->getOperand(1), DemandedEltsRHS));
  }
  if (!DemandedEltsLHS.isZero()) {
    return ComputeForSingleOpFunc(I->getOperand(0), DemandedEltsLHS);
  }
  return ComputeForSingleOpFunc(I->getOperand(1), DemandedEltsRHS);
```
I think, this should also address your objection regarding the identity.

What do you think?

https://github.com/llvm/llvm-project/pull/92429