[InstCombine][Docs] Update InstCombine contributor guide

[dtcxzyw]

Update the guideline to reduce the chance of miscompilation/performance regression.

[artagnon]

I thought the motivation for the samesign effort was to avoid canonicalizations like this, so that the back-end can choose?

[artagnon]

Is nsz meaningless for fcmp? I thought there are some optimizations already that take nsz on fcmp into account, with the property that -0. == +0.?

[dtcxzyw]

0.0 and -0.0 are considered equal, even if nsz is absent.

[artagnon]

Perhaps I'm confused about something? See CmpInst::isEquivalence, which is used in some places:

// Returns true if either operand of CmpInst is a provably non-zero
// floating-point constant.
static bool hasNonZeroFPOperands(const CmpInst *Cmp) {
  auto *LHS = dyn_cast<Constant>(Cmp->getOperand(0));
  auto *RHS = dyn_cast<Constant>(Cmp->getOperand(1));
  if (auto *Const = LHS ? LHS : RHS) {
    using namespace llvm::PatternMatch;
    return match(Const, m_NonZeroNotDenormalFP());
  }
  return false;
}

// Floating-point equality is not an equivalence when comparing +0.0 with
// -0.0, when comparing NaN with another value, or when flushing
// denormals-to-zero.
bool CmpInst::isEquivalence(bool Invert) const {
  switch (Invert ? getInversePredicate() : getPredicate()) {
  case CmpInst::Predicate::ICMP_EQ:
    return true;
  case CmpInst::Predicate::FCMP_UEQ:
    if (!hasNoNaNs())
      return false;
    [[fallthrough]];
  case CmpInst::Predicate::FCMP_OEQ:
    return hasNonZeroFPOperands(this);
  default:
    return false;
  }
}

[dtcxzyw]

equal is not equivalence. The latter one means one of the operands can be replaced by the other.

[nikic]

@artagnon Note that this does not actually checks the nsz flag, it checks whether the operands are non-zero. That's fine.

[nikic]

LGTM

[nikic]

@artagnon Note that this does not actually checks the nsz flag, it checks whether the operands are non-zero. That's fine.