[PATCH] D25691: [DAGCombiner] Add vector demanded elements support to computeKnownBits

[Bjorn Pettersson via llvm-commits]

bjope added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2057
+      // Known bits are the values that are shared by every demanded element.
       // TODO: support per-element known bits.
       KnownOne &= KnownOne2;
----------------
Shouldn't we remove this TODO now? Or is that a different idea about the possibility to fetch a set of individual known bits for each element in one request?
If originating value is a vector, then the caller could call computeKnownBits for one demanded element at a time. Doing it for all elements in one go will be compilcated since the recursion could be different for different elements.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2559
+    } else {
+      computeKnownBits(InVec, KnownZero, KnownOne, Depth + 1);
+    }
----------------
I suggest that you explicitly show that a decision was made to not track a specific element here. And maybe more importantly show that we can not use the original DemandedElts in the recursive call by selecting a new bit mask.
This could be done by making the recursion like this:
  computeKnownBits(InVec, KnownZero, KnownOne, APInt::getAllOnesValue(VecVT.getVectorNumElements()), Depth + 1);


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2568
   case ISD::BSWAP: {
     computeKnownBits(Op.getOperand(0), KnownZero2, KnownOne2, Depth+1);
     KnownZero = KnownZero2.byteSwap();
----------------
Any reason for not providing the DemandedElts in this recursive call?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2539
   case ISD::EXTRACT_VECTOR_ELT: {
     // At the moment we keep this simple and skip tracking the specific
     // element. This way we get the lowest common denominator for all elements
----------------
I guess this comment needs to be revised, or maybe removed.
And at least the TODO should be removed.


Repository:
  rL LLVM

https://reviews.llvm.org/D25691