[PATCH] D72007: [InstCombine] try to pull 'not' of select into compare operands

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D72007#1804065 <https://reviews.llvm.org/D72007#1804065>, @lebedev.ri wrote:

> Looks good to me in principle, the case where we get rid of `not` is good,
>  but i'm not sure about the case where we only manage to sink it into one of the hands.
>  To me that seems to be the opposite of the current general canonicalizaion we do.


The general 'not cmp' canonicalization is the same as this? See around line 3090:

  // not (cmp A, B) = !cmp A, B
  CmpInst::Predicate Pred;
  if (match(&I, m_Not(m_OneUse(m_Cmp(Pred, m_Value(), m_Value()))))) {
    cast<CmpInst>(Op0)->setPredicate(CmpInst::getInversePredicate(Pred));
    return replaceInstUsesWith(I, Op0);
  }



> (Do we need Inverter logic, that handles sinking of inversions as far as possible?)

That would be similar to D68408 <https://reviews.llvm.org/D68408> (did negator hit a roadblock?), so it could be an enhancement there.

> I agree that it is a throughput improvement for final assembly, but not sure about IR.
>  Is there some bigger pattern (of which this pattern is part of)
>  which shows why this is the correct IR canonicalization?

The app where I noticed this allowed eliminating the 'not' (the select had 2 cmps), but I've lost track of that larger example. So that case seems uncontroversial - we are able to remove an instruction.

I can reduce the scope of this patch to only handle that case, but then we will have a missing canonicalization for the more general case (1 cmp). Pulling the 'not' ahead of the select seems more likely to allow follow-on folding, so that's why I chose that as canonical. I can propose that separately or drop it if that's not a convincing argument.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72007/new/

https://reviews.llvm.org/D72007