[PATCH] D31311: [DAGCombiner] Add vector demanded elements support to ComputeNumSignBits

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3148
+    // an unknown element index, ignore DemandedElts and demand them all.
+    APInt DemandedElt = APInt::getAllOnesValue(NumSrcElts);
+    ConstantSDNode *ConstEltNo = dyn_cast<ConstantSDNode>(EltNo);
----------------
Nit: I'd use the plural (`DemandedElts`).


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3150
+    ConstantSDNode *ConstEltNo = dyn_cast<ConstantSDNode>(EltNo);
+    if (ConstEltNo && ConstEltNo->getAPIntValue().ult(NumSrcElts))
+      DemandedElt = APInt::getOneBitSet(NumSrcElts, ConstEltNo->getZExtValue());
----------------
Waiting for C++17… :-)


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3156
   case ISD::EXTRACT_SUBVECTOR:
     return ComputeNumSignBits(Op.getOperand(0), Depth + 1);
   case ISD::CONCAT_VECTORS:
----------------
Do you care about adding a TODO here?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3191
+    if (NumBits > 1)
+      FirstAnswer = std::max(FirstAnswer, NumBits);
   }
----------------
Was this change (the `if`, not the `NumBits`) done by `clang-format`?


================
Comment at: test/CodeGen/X86/known-bits-vector.ll:41
 ; X64-NEXT:    vmovq %xmm0, %rax
-; X64-NEXT:    vcvtsi2ssq %rax, %xmm2, %xmm0
+; X64-NEXT:    vcvtsi2ssl %eax, %xmm2, %xmm0
 ; X64-NEXT:    retq
----------------
I wouldn't expect tests to change when you're just avoiding work.
Is this test hitting the `computeKnownBits(...)` case at the end of `computeNumSignBits` and has a non-trivial `DemandedElts`? (non-trivial would mean "not all 1s")


Repository:
  rL LLVM

https://reviews.llvm.org/D31311