Page MenuHomePhabricator

[InlineCost] model calls to llvm.objectsize.*
Changes PlannedPublic

Authored by nickdesaulniers on Oct 8 2021, 12:00 PM.

Details

Summary

Very similar to https://reviews.llvm.org/D111272. We very often can
evaluate calls to llvm.objectsize.* regardless of inlining. Don't count
calls to llvm.objectsize.* against the InlineCost when we can evaluate
the call to a constant.

Link: https://github.com/ClangBuiltLinux/linux/issues/1302

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Oct 8 2021, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 12:00 PM
  • simplify test akin to as done according to feedback on D111272
nickdesaulniers planned changes to this revision.Oct 11 2021, 2:38 PM

Looking at https://llvm.org/docs/LangRef.html#id1293; there's a fourth parameter that says this check must be emitted at runtime. I should perhaps check that and bail early.

@craig.topper also mentioned that "it looks like maybe ObjectSizeOffsetVisitor doesn't know how to look through bitcasts?" on IRC. Let me look into that, because I suspect we should be able to evaluate call i64 @llvm.objectsize.i64.p0i8(i8* bitcast (%struct.nodemask_t* @numa_nodes_parsed to i8*), i1 false, i1 false, i1 false) to not--1.

kazu added a comment.Oct 11 2021, 5:22 PM

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?

void added a comment.EditedNov 17 2021, 3:45 PM

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. So when checking for "cost", we make a copy of the canonical version and then proceed as normal.

void added a comment.Nov 29 2021, 2:44 PM

@kazu, any thoughts on my comment?

kazu added a comment.Sep 13 2022, 12:54 PM

@kazu, any thoughts on my comment?

I like the idea of entering known correct values into SimplifiedValues. Now, when you say a separate SimplifiedValues object, do you mean maintaining something like SimplifiedValuesForCost and SimplifiedValuesForLegality?

Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 12:54 PM
Herald added a subscriber: ChuanqiXu. · View Herald Transcript