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

[Dhruv Chawla 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>


dc03-work wrote:

> MaxAnalysisRecursionDepth comes from LLVM-IR. The constant is not invented by GlobalIsel. We already use it in production:
> 
> https://github.com/llvm/llvm-project/blob/90e841131aa82560b6c7d1c1840bdfc79c091b37/llvm/lib/CodeGen/GlobalISel/Utils.cpp#L1887
> 
> I never heard on a GlobalIsel Combiner review discussions of data driven decisions of, e.g., MaxAnalysisRecursionDepth. Is 4,5,6, or 7 optimal for GlobalIsel. My feeling so far has been it is sufficient for new combines:
> 
> * tests for correctness (MIR)
> * code size improvement in end-to-end tests
> * [[GlobalIsel] Combine trunc of binop #107721](https://github.com/llvm/llvm-project/pull/107721) make existing code more efficient code to execute
> 
> If these rules have changed, we should move the discussion to #92309

The problem is neither the numeric value nor where it comes from. `KnownBits` analysis is expensive, period. I have profiled InstCombine in the past and KB analysis in functions like like `isKnownNonZero` and `SimplifyDemandedBits` take up a non-trivial amount of execution time, up to being multiple percent of the overall execution time of the pass. Unless the combine has a tangible improvement on the performance of benchmarks that offset the compile time cost, there is no good reason to add it.

Do also consider that GlobalISel is an instruction selector, not a middle-end pass like InstCombine. We don't need to do redo everything the middle-end does because the middle-end is supposed to do that for us :)

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