[PATCH] D111456: [InlineCost] model calls to llvm.objectsize.*

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D111456#3056647 <https://reviews.llvm.org/D111456#3056647>, @kazu wrote:

> Hmm.  I may be a bit pedantic, but please bear with me for a moment. Even though I've already LGTMed https://reviews.llvm.org/D111272, and this patch is quite similar in nature -- "speculative" folding, I think we could run into a problem for the following reason. `InlineCost.cpp` performs two tasks -- the legality check ("Can we inline?") and the desirability check ("Should we inline?").  Once we "speculatively fold" these intrinsics and update `SimplifiedValues`, we don't examine those "dead" basic blocks for legality purposes. This can be a problem if those basic blocks come back to life, and they contain un-inlinable instructions.  I suspect even the return value from `@llvm.objectsize` could change from the failure value (like -1) to the actual object size after ThinLTO importing.
>
> I am currently thinking about maintaining a parallel world `SimplifiedValues` and `LikelyValues` for operations that matter to folding conditional branches that check `@llvm.is.constant` and `@llvm.objectsize`.  I would also maintain a set of basic blocks that are "speculatively dead".  If I am scanning a basic block that is marked speculatively dead, then I check legality but do not increment `Cost`.
>
> `SimplifiedValues` appears 40 times in `InlineCost.cpp`, so it would be a nightmare to duplicate each one and maintain the perfect parallel world for little gain.
>
> Thoughts?

Why not have a separate `SimplifiedValues` object when checking for "cost"? There could be a canonical `SimplifiedValues` when checking legality, but when checking for cost, the only values that modify the canonical `SimplifiedValues` are those we *know* are correct---i.e., `llvm.is.constant` evaluates to `true`, or `llvm.objectsize` evaluates to a constant value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111456