[PATCH] D25200: [InstCombine] New opportunities for FoldAndOfICmp and FoldXorOfICmp

[David Majnemer via llvm-commits]

majnemer added inline comments.


> InstCombineAndOrXor.cpp:744-750
> +// For an ICMP where RHS is zero, we want to check if the ICMP is equivalent to
> +// comparing a group of bits in an integer value against zero. If yes we return
> +// (true, X, Mask). X is a boolean to identify condition code. Mask identifies
> +// BitGroup. This is mostly when LHS is 'and' of an integer with a mask, but
> +// there are other cases as well. Like when CC=SLT, where effectively we check
> +// to see if sign bit is one or not.
> +static std::tuple<bool, bool, const APInt *>

A tuple with multiple bool types can be a little confusing. I'd prefer a little struct. At the very least, this needs to be documented.

> InstCombineAndOrXor.cpp:751
> +static std::tuple<bool, bool, const APInt *>
> +IsAnyBitSet(Value *LHS, ICmpInst::Predicate CC) {
> +  auto Inst = dyn_cast<Instruction>(LHS);

Please follow the LLVM naming convention.

> InstCombineAndOrXor.cpp:752-754
> +  auto Inst = dyn_cast<Instruction>(LHS);
> +
> +  if (Inst && Inst->getOpcode() == Instruction::And) {

Prefer early return to reduce indentation.

> InstCombineAndOrXor.cpp:752-755
> +  auto Inst = dyn_cast<Instruction>(LHS);
> +
> +  if (Inst && Inst->getOpcode() == Instruction::And) {
> +    auto Mask = dyn_cast<ConstantInt>(Inst->getOperand(1));

Prefer `auto *`

> InstCombineAndOrXor.cpp:756-769
> +    if (!Mask)
> +      return std::make_tuple(false, false, nullptr);
> +
> +    switch (CC) {
> +    default: break;
> +    case ICmpInst::ICMP_EQ:
> +      return std::make_tuple(true, false, &(Mask->getValue()));

Prefer std::initialzier_list syntax here to reduce visual noise.

> InstCombineAndOrXor.cpp:762-765
> +      return std::make_tuple(true, false, &(Mask->getValue()));
> +    case ICmpInst::ICMP_NE:
> +    case ICmpInst::ICMP_UGT:
> +      return std::make_tuple(true, true, &(Mask->getValue()));

These parenthesis are not necessary.

> InstCombineAndOrXor.cpp:831-832
> +  Value *Val = LHS->getOperand(0), *Val2 = RHS->getOperand(0);
> +  ConstantInt *LHSCst = dyn_cast<ConstantInt>(LHS->getOperand(1));
> +  ConstantInt *RHSCst = dyn_cast<ConstantInt>(RHS->getOperand(1));
> +  if (!LHSCst || !RHSCst) return nullptr;

`auto *`

> InstCombineAndOrXor.cpp:833
> +  ConstantInt *RHSCst = dyn_cast<ConstantInt>(RHS->getOperand(1));
> +  if (!LHSCst || !RHSCst) return nullptr;
> +  ICmpInst::Predicate LHSCC = LHS->getPredicate(), RHSCC = RHS->getPredicate();

Please stick the return on its own line.

https://reviews.llvm.org/D25200