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

[Simon Pilgrim via llvm-commits]

RKSimon 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;
----------------
bjope wrote:
> 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.
> 
What that TODO is talking about is returning separate KnownZero and KnownOne values for every vector element and not combining them down to the 'shared' bits. Something that could end up requiring a significant refactor of this function and every caller.

This TODO was written when I still thought that would be necessary, but I haven't been able to make a strong enough case for that approach yet and just using the shared bits of demanded elts might be satisfactory for the cases I have so far.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2559
+    } else {
+      computeKnownBits(InVec, KnownZero, KnownOne, Depth + 1);
+    }
----------------
bjope wrote:
> 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);
This is what the computeKnownBits without the DemandedElts argument now does. I'll tweak the comments to make this clearer.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2568
   case ISD::BSWAP: {
     computeKnownBits(Op.getOperand(0), KnownZero2, KnownOne2, Depth+1);
     KnownZero = KnownZero2.byteSwap();
----------------
bjope wrote:
> bjope wrote:
> > Any reason for not providing the DemandedElts in this recursive call?
> Sorry, I just realised that BSWAP wasn't the only operation not providing DemandedElts in the recursive call.
> So I have no special interest in BSWAP. But I guess that there is a general TODO to pass DemandedElts in many more recursive calls.
As I mentioned in the summary, I've only added support for a few opcodes so far (the ones that have proven straightforward to test). The hope is that others can be added as they are required. I'd prefer to just add a TODO to the top of the function and to every outstanding opcode, unless you think it necessary?


================
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
----------------
bjope wrote:
> I guess this comment needs to be revised, or maybe removed.
> And at least the TODO should be removed.
No problem.


Repository:
  rL LLVM

https://reviews.llvm.org/D25691