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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D72007#1804132 <https://reviews.llvm.org/D72007#1804132>, @spatel wrote:

> 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);
>   }
>   
>   
>   


No, i'm not talking about clear-win cases where that allows to eliminate `not`/`neg`,
but about the cases where there is no change in instruction count.

>> (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>, so it could be an enhancement there.

I would personally say that *if* we have an Inverter, then generally, outside of it,
in the cases where there is no decrease in instruction count,
we should be consistently *hoisting* `not`/`neg`.
But clearly we are not there (yet?), so the solution is not as obvious.

> (did negator hit a roadblock?)

Yeah, it got too powerful and hit/exposed another conflicting canonicalization,
so i've stopped for now. I think it will need some kind of pattern blacklisting :/

>> 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.

The clear-win case LG; the other case where we only sink it, but not *necessarily* eliminate it,
while it doesn't look obviously-bad to me, it isn't immediately obvious to me that it is the correct direction.


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

https://reviews.llvm.org/D72007