[PATCH] D114766: If constrained intrinsic is replaced, remove its side effect

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In D114766#3161851 <https://reviews.llvm.org/D114766#3161851>, @kpn wrote:

> In D114766#3161806 <https://reviews.llvm.org/D114766#3161806>, @sepavloff wrote:
>
>> Probably the pass should run earlier, it is hard to reason without analyzing practical cases. If it is indeed so, we could move the pass or run it several times. Tuning pipeline is not unique for constant folding only. But without ability to remove instructions many optimizations do not make sense.
>
> But InstSimplify is used for analysis long before it is run for transformations. Other passes just call into InstSimplify and bypass InstSimplify's transformation pass machinery. Meaning, parts of InstSimplify do run earlier in the pipeline _already_.

As @nikic pointer out (https://reviews.llvm.org/D114766#3161692):

> However, mostly InstSimplify is only used as an analysis, with the most important consumer being InstCombine, which uses InstSimplify (the analysis, not the pass) and runs many times during the optimization pipeline.

Call attribute fixing occurs only in the pass, which runs once.

> And many if not most transformation passes eliminate instructions without going through InstSimplify. So tuning the pipeline is not the answer.

We do not eliminate instructions in InstSimplify. We mark them available for elimination. The elimination itself occurs in dedicated passes designed for just this purpose. So the question is only whether we may remove side effect from some calls.

> Coming up with a more general solution that can be used by multiple transformation passes is what we need.

The solution actually is the call `CI->addFnAttr(Attribute::ReadNone)`. Hardly there is a simpler or more general solution. We apply it when we know that the instruction is not needed anymore, also quite reasonable, why postpone this? The only possible concern is when we do the actions like constant folding, which may result in the attribute change. Does it occur too late? Or maybe it takes place at right moment? We do not know now. So I would propose to use with current state and change it when we found that there is a better solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114766