[PATCH] D59926: Constant folding sometimes creates large constants even though a smaller constant is used multiple times

[Ramakota Reddy via Phabricator via llvm-commits]

ramred01 added a comment.

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

> In D59926#1447460 <https://reviews.llvm.org/D59926#1447460>, @ramred01 wrote:
>
> > In D59926#1445936 <https://reviews.llvm.org/D59926#1445936>, @spatel wrote:
> >
> > > In D59926#1445839 <https://reviews.llvm.org/D59926#1445839>, @ramred01 wrote:
> > >
> > > > In D59926#1445823 <https://reviews.llvm.org/D59926#1445823>, @lebedev.ri wrote:
> > > >
> > > > > In D59926#1445822 <https://reviews.llvm.org/D59926#1445822>, @ramred01 wrote:
> > > > >
> > > > > > In D59926#1445820 <https://reviews.llvm.org/D59926#1445820>, @lebedev.ri wrote:
> > > > > >
> > > > > > > This sounds like a back-end problem.
> > > > > > >  Have you considered/tried solving it there, instead of limiting the middle-end?
> > > > > >
> > > > > >
> > > > > > This folding is happening in the middle-end itself.  The LLVM IR already has the folded value.
> > > > >
> > > > >
> > > > > That is precisely my point. Is that folding not optimal for the IR?
> > > >
> > > >
> > > > That folding would have been optimal for the IR if the constant were not to be reused.  If the constant is reused, then most architectures will do better with materializing one constant in a register and reusing it rather than materializing two constants.  Most architectures require two instructions to materialize 32-bit constants.  If, we add the additional operation of shift also, that now needs to be done, even with one reuse, it will generate 1 instruction fewer.  With more reuses and each reuse folded, that number could increase.
> > >
> > >
> > > I think you're mixing up canonicalization with optimization (which I've also done a bunch). The goal here in instcombine is to produce canonical code. Ie, create a reduced set of easier-to-optimize forms of the infinite patterns that might have existed in the source. Canonical code often is identical to optimal code, but sometimes they diverge. That's when things get harder (and we might end up reversing an earlier transform). But we have to deal with it in the backend because -- as Roman noted -- any solution at this level would be incomplete simply because the input might already be in the form that you don't want.
> >
> >
> > I get your point and fully agree with it.
> >
> > But if a certain canonicalization were to result in a form that always requires reversing that transform at a later stage, won't we be better off not performing that transform in the first place?  If a canonicalization were to result in exposing better optimization opportunities across architectures, then that makes more sense, isn't it?
>
>
> Is it *always*, though?
>  If i understand correctly, you want to block this transform, right?
>  https://godbolt.org/z/IvWYKl
>
> we have //essentially// replaced
>
>   %4 = ashr i32 %3, 8
>   %5 = icmp slt i32 %4, 4096
>   
>
> with
>
>   %5 = icmp slt i32 %3, 1048832
>   
>
> thus that cmp does not depend on the `ashr`, thus we reduced the data dependency chain,
>  thus `icmp` can execute without waiting for the `ashr`.
>
> Which is, as it can be seen from the lowest view there, unsurprisingly improves performance. (assuming i fixed-up asm for mca correctly..)
>
> So i'm not convinced that this hammer approach is the right solution to the problem.


Yes, that is exactly what I am trying to block.

What I have missed out is that this should be applicable only for -Os and -Oz.  When performance is the criteria, then the folding is perfectly fine.  But when size is the criteria, then across architectures, the problem remains the same, since you cannot materialize a 32-bit constant in less than two instructions.  The only exceptions being the powers of two in certain architectures.  So each new constant that is created adds two instructions to the size if we perform constant folding on a constant that has multiple uses and create a new constant for each such use.


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

https://reviews.llvm.org/D59926