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

[Amara Emerson 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>


aemerson wrote:

> CombinerHelper::matchCmpOfZero uses directly KB at most once per case in the switch statement of the icmp predicate, for reference:
> 
> https://github.com/llvm/llvm-project/blob/ce9f9872950090eae9104ff49b77469e3dc0a3c5/llvm/lib/Analysis/InstructionSimplify.cpp#L2974
> 
> A different question is the isKnownNonZero analysis, but it is depth limited: `bool isKnownNonZero(Register Reg, const MachineRegisterInfo &MRI, GISelKnownBits *KB, unsigned Depth = 0);`

MaxAnalysisRecursionDepth is defaulted to 6. We're going to potentially do a 6-deep KB analysis on every icmp. Maybe we don't need to start with that and only use it if the data supports it's worth it from code size or performance metrics? Have you run those benchmarks? Or do I need to do it myself again?

I'm going to level with you here: at this point in the conversation any time you bring up an IR combine for reference to use is not helping your case in the way you think. I really suggest you take a step back from this and consider why we have such resistance to this change.

You basically have a lot of code, and you're resisting splitting up the visitICmp() function on the basis of efficiency, but you're also ok with adding a KB dependent combine without also having data about it's cost/benefit. We need a fundamental rethink in *how* we add changes to GlobalISel. Individual patches like these are not important. They really aren't. Setting good examples and precedents, along with pragmatic choices backed up by data are what's important.

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