[PATCH] D70462: [InstCombine] Change InstCombineAddSub to not perform constant folding when there is an intermediate use of the source register.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D70462#1753932 <https://reviews.llvm.org/D70462#1753932>, @dancgr wrote:

> In D70462#1752506 <https://reviews.llvm.org/D70462#1752506>, @lebedev.ri wrote:
>
> > -1 to adding more artificial one-use restrictions.
> >  I agree there is a general problem of unability to see if
> >  there exists a particular instruction already.
>
>
> I'm wondering why we should avoid one-use restrictions. Is there any discussion that I'm not aware of?


I'm not aware of any concise write-up, but there are at least two things i can note:
Given

  %t0 = add i32 %arg, 8
  call void @use(i32 %t0)
  %t1 = sub i32 2, %t0

That we currently transform into

  %t0 = add i32 %arg, 8
  call void @use(i32 %t0)
  %t1 = sub i32 -6, %arg

1. `%t1` no longer depends on `%t0`, which means `%t1` does not have to wait until `%t0` finishes executing - reduced dependency chain, improved computation parallelizm
2. We reduced use-count of `%t0`. There may be some other transform involving `%t0` that was previously blocked because `%t0` had other uses, which now may happen.

> In this case, we only want to apply the folding if there is only one use of first expression.

More specifically, by introducing such limitation we no longer fold this pattern in tests,
which leaves extra instruction[s] which allows the motivational fold in the patch's description to happen.
(please add that test)

> If there is any more than that, the folding won't give any improvement or cause issues with other optimizations.

How do we know that?

> How would you suggest that I avoid this issue without checking for one-use?

I don't have a simple solution for the issue as described in the patch's description,
although i feel like noting that i believe i have seen similar cases myself.

This is generally impossible to solve with the existing instcombine design - it would need
to be something more similar to what VPlan is supposed to end up being - a list of
possible transforms that we can apply, from which we'd pick so that we end up with best results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70462