Skip to content

[InlineCost] Don't call collectEphemeralValues() if too many assumptions #129279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[InlineCost] Don't call collectEphemeralValues() if too many assumptions #129279

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion llvm/lib/Analysis/InlineCost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,12 @@ static cl::opt<bool> DisableGEPConstOperand(
"disable-gep-const-evaluation", cl::Hidden, cl::init(false),
cl::desc("Disables evaluation of GetElementPtr with constant operands"));

static cl::opt<unsigned> EphValAssumptionsSzLimit(
"inline-ephval-assumptions-limit", cl::Hidden, cl::init(5000),
cl::desc("Collection of ephemeral values can be very expensive "
"compile-time wise, so don't collect them if the function "
"contains more than this many assumptions"));

namespace llvm {
std::optional<int> getStringFnAttrAsInt(const Attribute &Attr) {
if (Attr.isValid()) {
Expand Down Expand Up @@ -2785,7 +2791,15 @@ InlineResult CallAnalyzer::analyze() {
// the ephemeral values multiple times (and they're completely determined by
// the callee, so this is purely duplicate work).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to resolve this FIXME instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be ideal. Caching the EphValues set based on the callee would probably fix the issue. The problem is that I am not sure I will be able to catch all cases where caching would need to be invalidated. I don't see any tests for this code which would make it tricky to validate.

SmallPtrSet<const Value *, 32> EphValues;
CodeMetrics::collectEphemeralValues(&F, &GetAssumptionCache(F), EphValues);
auto &AC = GetAssumptionCache(F);
// Don't collect ephemeral values if we have too many assumptions because the
// collection can be very expensive. Note that this will increase the cost of
// the function, making it less likely to be inlined. But if the function
// contains thousands of assumptions, then the chances are that it's too large
// to inline anyway.
bool TooExpensive = AC.assumptions().size() > EphValAssumptionsSzLimit;
if (!TooExpensive)
CodeMetrics::collectEphemeralValues(&F, &AC, EphValues);

// The worklist of live basic blocks in the callee *after* inlining. We avoid
// adding basic blocks of the callee which can be proven to be dead for this
Expand Down
Loading