[PATCH] D31249: [DAGCombiner] Add vector demanded elements support to computeKnownBitsForTargetNode

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: include/llvm/Target/TargetLowering.h:2416
   /// or one and return them in the KnownZero/KnownOne bitsets.
   virtual void computeKnownBitsForTargetNode(const SDValue Op,
                                              APInt &KnownZero,
----------------
Do we need to describe the DemandedElts parameter here? 

One thing is that the SelectionDAG::computeKnownBits method currently does the check if DemandedElts is empty. So the method is not supposed to be called with an empty DemandedElts. Is this something that all the target implementations should rely on? Otherwise I guess you need to add the same kind of early-out for each target implementation? (Or is it correct to answer with a set of known bits even if the request was for no element?)
Not really sure if it helps to add a comment about that here, but I guess we have no way to really encode that DemandedElts should be non-empty. Another approach might be to add asserts in every target implementation that DemandedElts isn't empty.



================
Comment at: lib/Target/AMDGPU/AMDGPUISelLowering.cpp:3556
+    const SDValue Op, APInt &KnownZero, APInt &KnownOne,
+    const APInt &DemandedElts, const SelectionDAG &DAG, unsigned Depth) const {
 
----------------
It would have been a smaller diff to review if only adding the line with DemandedElts. Now I needed to understand if something else changed as well here due to mixing cleanup and adding functionality in the same patch.
(But maybe this is something that clang-format did for you?)

Anyway, the change looks OK so this is just a minor remark.




Repository:
  rL LLVM

https://reviews.llvm.org/D31249