Skip to content
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.

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

LoopUnroll fails to unroll forced loop due to inlinable calls, leading to linker errors #88141

Open
gburgessiv opened this issue Apr 9, 2024 · 0 comments · May be fixed by #88149
Open

LoopUnroll fails to unroll forced loop due to inlinable calls, leading to linker errors #88141

gburgessiv opened this issue Apr 9, 2024 · 0 comments · May be fixed by #88149
Assignees

Comments

@gburgessiv
Copy link
Member

An AFDO profile roll broke Chromium's build with ThinLTO enabled by introducing linker warnings - which were promoted to errors - about llvm.loop.unroll.enabled loops not being unrolled:

ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-optimization-reducer.h:685:11: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:1316:9: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:813:9: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:2124:9: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:2149:13: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:2154:20: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-lowering-reducer-inl.h:2185:20: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
ld.lld:  [0;31merror:  [0m../../../../../../../home/chrome-bot/chrome_root/src/v8/src/compiler/turboshaft/machine-optimization-reducer.h:685:11: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Unfortunately, I can't provide an 'easy' reproducer of the original breakage, as it only occurs with ThinLTO and a specific AutoFDO profile on a very large program.

What I can provide is some information about investigating this locally:

  1. The LoopUnroll pass ran on the loops that did not get unrolled.
  2. The loops that did not get unrolled did have llvm.loop.unroll.enable metadata on them.
  3. The loops that did not get unrolled had call(s) to functions that were deemed inline candidates by UnrollCostEstimator. Moreover, the following criteria were met for at least one call per loop (PrepareForLTO was false):
if (!Call->isNoInline() && TTI.isLoweredToCall(F) &&
     ((F->hasInternalLinkage() && F->hasOneLiveUse()) || PrepareForLTO)) {
  // ...
}

and unrolling is currently always skipped if that's detected in a loop:

  if (UCE.NumInlineCandidates != 0) {
    LLVM_DEBUG(dbgs() << "  Not unrolling loop with inlinable calls.\n");
    return LoopUnrollResult::Unmodified;
  }

I can totally understand the desire to facilitate the inlining of calls like this, but if a user asks for forced unrolling, IMO that should be respected regardless.

I have a potential fix that I'll upload soon.

@gburgessiv gburgessiv self-assigned this Apr 9, 2024
gburgessiv added a commit to gburgessiv/llvm-project that referenced this issue Apr 9, 2024
`NumInlineCandidates` counts candidates that are _very likely_ to be
inlined. This is a useful metric, but causes linker warnings if:
- the loop to be unrolled has had unrolling forced by the user, and
- the inliner fails to inline the call (e.g., because it's considered a
  very cold callsite)
gburgessiv added a commit to gburgessiv/llvm-project that referenced this issue May 10, 2024
`NumInlineCandidates` counts candidates that are _very likely_ to be
inlined. This is a useful metric, but causes linker warnings if:
- the loop to be unrolled has had unrolling forced by the user, and
- the inliner fails to inline the call (e.g., because it's considered a
  very cold callsite)

Fixes llvm#88141
gburgessiv added a commit to gburgessiv/llvm-project that referenced this issue May 20, 2024
`NumInlineCandidates` counts candidates that are _very likely_ to be
inlined. This is a useful metric, but causes linker warnings if:
- the loop to be unrolled has had unrolling forced by the user, and
- the inliner fails to inline the call (e.g., because it's considered a
  very cold callsite)

Fixes llvm#88141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants