[PATCH] D142300: [InstSimplify] Simplify `icmp eq/ne/ugt/ult X, 0` -> true/false where `X` is known to be zero.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D142300#4071621 <https://reviews.llvm.org/D142300#4071621>, @nikic wrote:

> This doesn't make sense to me. If the value is known all zeroes, then the value should fold to zero, at which point the icmp becomes trivial (not necessarily in InstSimplify, but in InstCombine).
>
> The basic pattern from your tests already folds to zero: https://llvm.godbolt.org/z/EWTd4P3aY The pattern with the zero comparison does not: https://llvm.godbolt.org/z/xnv4joe9G
>
> At a guess, this is because we specially handle values used inside return instructions in https://github.com/llvm/llvm-project/blob/8e7c1b9a3d5f08046261ca0b7e5d319a4c4c9b25/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L3111 -- we should possibly stop doing that, because it gives a misleading picture of which folds work and which don't.
>
> Ands that fold to constants based on known bits are intended to reliably fold as part of demanded bits simplification here: https://github.com/llvm/llvm-project/blob/8e7c1b9a3d5f08046261ca0b7e5d319a4c4c9b25/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L200
>
> The problem is that the pattern you are testing is special-cased in ValueTracking only: https://github.com/llvm/llvm-project/blob/8e7c1b9a3d5f08046261ca0b7e5d319a4c4c9b25/llvm/lib/Analysis/ValueTracking.cpp#LL1088C75-L1088C75 Because demanded bits simplification reimplements the known bits calculation for performance reasons, this is lost in that case.
>
> The right fix here would be to make sure that the special-case known bits calculation for and is shared between ValueTracking and SimplifyDemanded.

I see. That makes sense and will do for v2 (was very confused why it works for cmp against non-zero, but not against zero <https://llvm.godbolt.org/z/783rv1o49>). Any chance you can review `D142271` first? I think the way to do this is to factor out a helper method and would be nice to get both patterns / not have inter-patch dependencies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142300