[PATCH] D72944: [InstCombine] Fix worklist management when simplifying demanded bits (PR44541)

Mon Jan 20 05:30:59 PST 2020

nikic added a comment.

In D72944#1829322 <https://reviews.llvm.org/D72944#1829322>, @spatel wrote:

> To fix the infinite loop bug without this change, we could add a clause in canonicalizeMinMaxWithConstant() to avoid creating this:
>  %cmp = icmp sgt i16 0, %sub
>
> Ie, if the LHS returned by matchSelectPattern() is a constant, swap LHS and RHS because we know we can't create a canonical compare that way. An alternative would be to do that swap within matchSelectPattern() itself. We already have a different LHS/RHS constraint for ABS/NABS on that analysis.
>
> I'm not sure how we can expose that bug with this fix in place though, so might want to make that fix first to be safer?

I don't think this would help. The problem here is not that the icmp operand order is non-canonical (though it would of course be better if we directly created it in canonical order), but that the (SPF) canonical form is based on `%sub` rather than `%arg`. This is because SPF has special support for sub-based min/max in https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/lib/Analysis/ValueTracking.cpp#L4687-L4699. I think to avoid this issue interdependently of this patch, we'd have to special case that code to not match the degenerate case where the `sub` has a zero on the RHS.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72944