[PATCH] D75801: [InstCombine] Remove known bits constant folding (WIP)

Fri Mar 20 03:46:03 PDT 2020

nikic marked an inline comment as done.
nikic added a comment.

In D75801#1923012 <https://reviews.llvm.org/D75801#1923012>, @lebedev.ri wrote:

> In D75801#1912306 <https://reviews.llvm.org/D75801#1912306>, @lebedev.ri wrote:
>
> > Do we do this fold somewhere else in the pipeline?
>
>
> ?
>  Do we believe CVP/SCCP/??? will cover all the possible cases?

I believe this should be covered by two vectors: First, by known bits calculation as part of InstCombineSimplifyDemanded. We don't call this for literally everything, but it handles all the cases that are likely to be fully known via known bits (bitwise ops and shifts as root instructions). Second, we have the same known bits optimization as here also as part of SimplifyInstruction. That means we already perform this in all places that use that high-level interface (e.g. it happens during inlining). I do want to get rid of that known bits call as well, but I think it is the reason why the InstCombine fold triggers exactly zero times on test-suite.

In D75801#1926700 <https://reviews.llvm.org/D75801#1926700>, @spatel wrote:

> http://llvm-compile-time-tracker.com/?config=O3&stat=wall-time
>  That looks great!
>  Is that end-to-end time for the entire compile or just 'opt -O3'?

Those are end-to-end times. Wall time is a bit too noisy to be really useful, so I tend to look at `instructions`, which is a reasonable proxy, and accurate to within `0.1%` for most benchmarks.

================
Comment at: test/Analysis/ValueTracking/known-signbit-shift.ll:46
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[B:%.*]], i32 2147221504, i32 2146959360
+; CHECK-NEXT:    ret i32 [[SEL]]
 ;
----------------
I missed these Analysis test changes before. The shl here is poison because we know it wraps based on known bits. This optimization gets lost now.

I could add it back explicitly (and better, by returning undef rather than zero) like this: https://gist.github.com/nikic/29135f304f7cf9de6d18dff7ca12659a

I'm not sure whether that's worthwhile though, it seems that these tests are more about not crashing due to conflicting known bits than anything else.

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

https://reviews.llvm.org/D75801