[PATCH] D31311: [DAGCombiner] Add vector demanded elements support to ComputeNumSignBits
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 24 12:07:17 PDT 2017
RKSimon 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);
----------------
filcab wrote:
> Nit: I'd use the plural (`DemandedElts`).
Except it will cause a Wshadow problem - I can use DemandedElts maybe?
================
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());
----------------
filcab wrote:
> Waiting for C++17… :-)
Someday......
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3156
case ISD::EXTRACT_SUBVECTOR:
return ComputeNumSignBits(Op.getOperand(0), Depth + 1);
case ISD::CONCAT_VECTORS:
----------------
filcab wrote:
> Do you care about adding a TODO here?
I'd prefer not to - almost every opcode in this function needs a TODO right now.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3191
+ if (NumBits > 1)
+ FirstAnswer = std::max(FirstAnswer, NumBits);
}
----------------
filcab wrote:
> Was this change (the `if`, not the `NumBits`) done by `clang-format`?
I'll do a NFCI clang-format cleanup.
================
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
----------------
filcab wrote:
> filcab wrote:
> > 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")
> Nevermind. This can happen on the other ones, when the minimum won't be among as many elements as before.
> Assuming this test uses the `EXTRACT_VECTOR_ELT`, we probably are missing one for `BUILD_VECTOR`.
It is actually calling the computeNumSignBits BUILD_VECTOR case here, but I'll add another test to make it clearer.
Repository:
rL LLVM
https://reviews.llvm.org/D31311
More information about the llvm-commits
mailing list