User Details
- User Since
- Aug 7 2020, 3:53 PM (99 w, 3 d)
Mon, Jun 20
Fri, Jun 17
Wed, Jun 15
May 18 2022
I've been only using it for development as a way to stabilize inlining when comparing compiler changes that impact function size. That way, we can better examine the impact of the change rather than the resulting inlining differences. It can certainly be used in production but I don't know of a user doing so.
Apr 27 2022
Checking locally I'm seeing LF as the line ending in crlf.cpp in the working directory. Can you double check that everything matches up?
Mar 16 2022
Mar 14 2022
Mar 11 2022
Clang-12 maintains dso_local on f1 as well, clang-13 and trunk lose it: https://godbolt.org/z/oejTzWsoW
Mar 10 2022
LGTM aside from one comment. Thanks for splitting this out!
Internally in LLVM IR processing interposability is represented by the following check:
bool GlobalValue::isInterposable() const { if (isInterposableLinkage(getLinkage())) return true; return getParent() && getParent()->getSemanticInterposition() && !isDSOLocal(); }
Mar 9 2022
Looking through the ELF test cases and changes:
- ELF/lto/internalize-basic.ll and ELF/lto/internalize-exportdyn.ll are simple test cases that should work identically on MachO to test this case.
- For the ELF definition of FinalDefinitionaInLinkageUnit:
r.FinalDefinitionInLinkageUnit = (isExec || sym->visibility != STV_DEFAULT) && dr && // Skip absolute symbols from ELF objects, otherwise PC-rel relocations // will be generated by for them, triggering linker errors. // Symbol section is always null for bitcode symbols, hence the check // for isElf(). Skip linker script defined symbols as well: they have // no File defined. !(dr->section == nullptr && (!sym->file || sym->file->isElf()));
Feb 23 2022
LGTM
LGTM
Feb 17 2022
The Windows build testing is failing. Took a look and pretty sure you just need to rebase past 19bdf44d850884a13a8708ccf1260fb7f4ef4eb3 which reverted the patch with the failing tests.
Dec 28 2021
Dec 26 2021
Dec 23 2021
Remove variable backend version check
Formatting
Nov 30 2021
Nov 18 2021
Condense lines
Nov 17 2021
Nov 12 2021
Gentle ping @bruno
Nov 9 2021
Add driver test
Remove whitespace change
Oct 29 2021
Thanks for the reviews @mtrofin !
Rebase and add -disable-output consistently to inline-replay.ll
Oct 28 2021
Move more things to ReplayInlineAdvisor.h and other feedback
Thanks, I'll make sure to avoid pointing at directories to validate these things in the future.
Oct 27 2021
Fix up comment
ReplayMode->ReplayFallback, refactoring
Review feedback
Oct 21 2021
Add comment to SampleProfileLoader::getInlineCandidate. Add inline-topdown-missing.prof file that wasn't part of the diff before.
@wenlei I refactored the implementation so that all queries to the external advisor route to getExternalInlineAdvisorCost or getExternalInlineAdvisorShouldInline depending on if we want the actual cost or just if inlining occurs, respectively. This unifies every location that queries for the existence of remarks (the original one in hasInlineAdvice and the new ones that add sites as candidates) and also means I don't need to expose a hasRemarksForFunction from the ReplayAdvisor and can do everything using the original InlineAdvisor interface. I also edited how importing is done:
Refactor uses of replay advisor into getExternalInlineAdvisorCost and getExternalInlineAdvisorShouldInline. Move importing code to findExternalInlineCandidate
To keep the conversation coherent I'll talk about the same feedback for D87551 here:
Oct 18 2021
Use getCanonicalFnName, add same path to inlineHotFunctionsWithPriority
Oct 13 2021
Fix up description so to be consistent in that Function scope is default.
Oct 12 2021
Ah, I'm thinking ""will not be inlined into" vs. "inlined into" in the remarks is the differentiation for positive/negative as you suggested before. Thinking further, in that setup only strict-negative and strict-positive would exist as modifiers. If implemented we would drop the strict naming completely: unspecified would mean leave it to the heuristic and positive/negative would mean always not inline/inline call sites without remarks associated with them.
@mtrofin Adding the enum ReplayInlineScope revealed that I was adding #include "llvm/Analysis/ReplayInlineAdvisor.h" to InlineAdvisor.cpp which seems wrong from a dependency perspective. Looking at how the other advisors interact it seems the interface is to declare a getReplayInlineAdvisor in InlineAdvisor.h similar to getReleaseModeAdvisor and also define ReplayInlineScope there so including InlineAdvisor.h by itself is enough to be able to create a ReplayInlineAdvisor. Does this look right?
Review feedback. InlineCallersFromRemarks->CallersToReplay. Refactor creating a ReplayInlineAdvisor to use getReplayInlineAdvisor.
Oct 11 2021
Address feedback and change over to replay-scope