[llvm] [GlobalIsel] Visit ICmp (PR #105991)

[Nikita Popov via llvm-commits]

Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>,
Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>,
Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>,
Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>,
Thorsten =?utf-8?q?Schütt?= <schuett at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/105991 at github.com>


nikic wrote:

I see at least another unrelated undef combine in here, but I was more talking about the entire infrastructure being added here. You may technically be adding one "combine", but the underlying analysis has lots of cases, most of which do not appear to be tested, and where I seriously doubt the need to replicate them in GlobalISel. This PR has very exotic stuff like "`x | (x != 0)` is non-zero" -- is this *really* important for the simplification of legalization artifacts?

I'm sure that there is some subset of the simplifications proposed here that is indeed useful, but I don't think that the PR as a whole, as you have presented is, is appropriate.

The general approach of "take some methods from InstSimplify and copy them to GlobalISel" is something I very much want to discourage. If you want to copy simplifications from somewhere, at least look at DAGCombine instead, where there is at least a chance that somebody has exercised some degree of due diligence when adding things there. But in an case, any handling you add needs to have test coverage, including any cases in underlying analyses.

https://github.com/llvm/llvm-project/pull/105991