Page MenuHomePhabricator

modimo (Di Mo)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 7 2020, 3:53 PM (99 w, 3 d)

Recent Activity

Mon, Jun 20

modimo committed rG523adafbd252: [test][AlwaysInline]:Correct comment and file check for always-inline.ll (authored by iamarchit123).
[test][AlwaysInline]:Correct comment and file check for always-inline.ll
Mon, Jun 20, 4:54 PM · Restricted Project, Restricted Project
modimo closed D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll.
Mon, Jun 20, 4:53 PM · Restricted Project, Restricted Project

Fri, Jun 17

modimo accepted D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll.
Fri, Jun 17, 11:56 AM · Restricted Project, Restricted Project

Wed, Jun 15

modimo added inline comments to D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll.
Wed, Jun 15, 5:34 PM · Restricted Project, Restricted Project
modimo added a reviewer for D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll: hoy.
Wed, Jun 15, 2:33 PM · Restricted Project, Restricted Project

May 18 2022

modimo added a comment to D125495: [Inline][Remark] Annotate inline pass name with link phase information for analysis..

Re replay inline, the pass name for replay inline advices will remain the same with -annotate-inline-lto-phase on -> the replay inline advisor class implementation remain the same before and after this change.
For my understanding, is the replay inline advisor used for development (conveniently inline the functions according to an external file)? Is the replay inline advisor used for production build?

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.

May 18 2022, 2:23 PM · Restricted Project, Restricted Project

Apr 27 2022

modimo closed D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 6:43 PM · Restricted Project, Restricted Project
modimo accepted D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 6:43 PM · Restricted Project, Restricted Project
modimo reopened D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 6:43 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

I used arc patch and also saw the same thing.

The patch does actually change the files to LF endings. So just applying the patch with non-Git tools will make LF endings, but Git will apply the LF -> CRLF transformation when it checks out itself. Git doesn't show the file as modified because after cleaning the file (i.e. applying CRLF -> LF) it's the same as in the index.

Apr 27 2022, 6:42 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

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?

I had this too, but checking out again seems to have fixed it. Maybe you used arc patch like I did? I presume that applies just textually and doesn't know about .gitattributes.

And sorry for landing already, there seems to have been a race...

Apr 27 2022, 6:13 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

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?

Apr 27 2022, 6:11 PM · Restricted Project, Restricted Project
modimo requested changes to D124563: Drop '* text=auto' from .gitattributes and normalize.

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?

Apr 27 2022, 6:04 PM · Restricted Project, Restricted Project
modimo accepted D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 5:59 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

Drop * text=auto, so that we renormalize only the files that need it.

Apr 27 2022, 5:55 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

I think the way to go is to revert ac5f7be6a868 then land everything as a single stack to prevent this issue.

Doesn't change anything about existing commits though. There is no way to fix that now I'm afraid. We can only fix it for future commits.

Apr 27 2022, 5:48 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

If I check out this commit and then check out the previous commit, clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp and clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected become modified in my working directory; their line endings are changed from CRLF to LF. That seems undesirable.

Apr 27 2022, 4:59 PM · Restricted Project, Restricted Project
modimo added a comment to D124563: Drop '* text=auto' from .gitattributes and normalize.

The following files have their line endings (when checked out on disk) changed from CRLF to LF by this patch. Seems harmless, but I just wanted to confirm that it was expected:

clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include2.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-if-constexpr.cpp
clang-tools-extra/test/modularize/Inputs/CompileError/module.modulemap
clang-tools-extra/test/modularize/Inputs/MissingHeader/module.modulemap
clang-tools-extra/test/pp-trace/Inputs/module.map

(The files which should be CRLF according to .gitattributes remained CRLF.)

Apr 27 2022, 4:52 PM · Restricted Project, Restricted Project
modimo retitled D124563: Drop '* text=auto' from .gitattributes and normalize from renormalize to Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b.
Apr 27 2022, 3:24 PM · Restricted Project, Restricted Project
modimo updated the summary of D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 3:21 PM · Restricted Project, Restricted Project
modimo requested review of D124563: Drop '* text=auto' from .gitattributes and normalize.
Apr 27 2022, 3:16 PM · Restricted Project, Restricted Project

Mar 16 2022

modimo accepted D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll.
Mar 16 2022, 12:16 PM · Restricted Project, Restricted Project, Restricted Project

Mar 14 2022

modimo accepted D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable.
Mar 14 2022, 4:29 PM · Restricted Project, Restricted Project, Restricted Project

Mar 11 2022

modimo added a comment to D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable.

Clang-12 maintains dso_local on f1 as well, clang-13 and trunk lose it: https://godbolt.org/z/oejTzWsoW

Mar 11 2022, 4:07 PM · Restricted Project, Restricted Project, Restricted Project
modimo added a comment to D119506: [lld-macho] Set FinalDefinitionInLinkageUnit on most LTO externs.

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.

The cases in internalize-basic.ll are covered by our lto-internalize.ll. internalize-exportdyn.ll is mostly covered by our lto-internalize-unnamed-addr.ll. (We use dylibs instead of executables linked with the -export_dynamic, but the code paths tested are basically identical, since that flag makes executable symbols get treated like dylib ones.) I did notice two missing cases, however: we missed out testing the case where a symbol is sometimes linkonce_odr and sometimes weak_odr, and we were only checking the visibility of the symbols in the final output binary, whereas the ELF tests were checking the visibility of the symbols at the IR level (after the internalize stage of LTO). I've put up D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll to cover those cases.

Mar 11 2022, 3:29 PM · Restricted Project, Restricted Project, Restricted Project
modimo updated subscribers of D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll.
Mar 11 2022, 2:53 PM · Restricted Project, Restricted Project, Restricted Project

Mar 10 2022

modimo accepted D121351: [lld-macho][nfc] Allow Defined symbols to be placed in binding sections.

LGTM aside from one comment. Thanks for splitting this out!

Mar 10 2022, 1:54 PM · Restricted Project, Restricted Project, Restricted Project
modimo added inline comments to D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable.
Mar 10 2022, 1:50 PM · Restricted Project, Restricted Project, Restricted Project
modimo added a comment to D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable.

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 10 2022, 1:50 PM · Restricted Project, Restricted Project, Restricted Project

Mar 9 2022

Herald added a project to D119294: [lld-macho] -flat_namespace for dylibs should make all externs interposable: Restricted Project.
Mar 9 2022, 1:13 PM · Restricted Project, Restricted Project, Restricted Project
Herald added a project to D119506: [lld-macho] Set FinalDefinitionInLinkageUnit on most LTO externs: Restricted Project.

Looking through the ELF test cases and changes:

  1. 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.
  2. 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()));
Mar 9 2022, 1:05 PM · Restricted Project, Restricted Project, Restricted Project

Feb 23 2022

modimo accepted D120095: Simplify/cleanup BasicBlockUtilsTest.

LGTM

Feb 23 2022, 1:13 PM · Restricted Project
modimo accepted D120096: PGOInstrumentation, GCOVProfiling: Split indirectbr critical edges regardless of PHIs.

LGTM

Feb 23 2022, 1:10 PM · Restricted Project

Feb 17 2022

modimo added inline comments to D120096: PGOInstrumentation, GCOVProfiling: Split indirectbr critical edges regardless of PHIs.
Feb 17 2022, 5:56 PM · Restricted Project
modimo added a comment to D120095: Simplify/cleanup BasicBlockUtilsTest.

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.

Feb 17 2022, 5:20 PM · Restricted Project

Dec 28 2021

modimo committed rGba51d26ec451: [CodeView] Clamp Frontend version (authored by modimo).
[CodeView] Clamp Frontend version
Dec 28 2021, 3:22 PM
modimo closed D116243: [CodeView] Clamp Frontend version.
Dec 28 2021, 3:22 PM · Restricted Project
modimo added a comment to D116243: [CodeView] Clamp Frontend version.

Out of curiosity do you know if S_COMPILE3 requires each version field to be 16-bits or can it be larger?

It needs to be 16-bits, please see: https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/include/cvinfo.h#L3361

Dec 28 2021, 2:47 PM · Restricted Project

Dec 26 2021

modimo added a comment to D116243: [CodeView] Clamp Frontend version.

LGTM, thanks!

Dec 26 2021, 5:12 PM · Restricted Project

Dec 23 2021

modimo updated the diff for D116243: [CodeView] Clamp Frontend version.

Remove variable backend version check

Dec 23 2021, 3:08 PM · Restricted Project
modimo updated the diff for D116243: [CodeView] Clamp Frontend version.

Formatting

Dec 23 2021, 2:18 PM · Restricted Project
modimo updated the summary of D116243: [CodeView] Clamp Frontend version.
Dec 23 2021, 2:15 PM · Restricted Project
modimo retitled D116243: [CodeView] Clamp Frontend version from [CodeView] Clamp Frontend Version to [CodeView] Clamp Frontend version.
Dec 23 2021, 2:14 PM · Restricted Project
modimo requested review of D116243: [CodeView] Clamp Frontend version.
Dec 23 2021, 2:11 PM · Restricted Project

Nov 30 2021

modimo committed rG47f230ba2c8f: Add toggling for -fnew-infallible/-fno-new-infallible (authored by modimo).
Add toggling for -fnew-infallible/-fno-new-infallible
Nov 30 2021, 5:35 PM
modimo closed D113523: Add toggling for -fnew-infallible/-fno-new-infallible.
Nov 30 2021, 5:35 PM · Restricted Project
modimo committed rG9b704d31b54a: [Clang] Add option to disable -mconstructor-aliases with -mno-constructor… (authored by modimo).
[Clang] Add option to disable -mconstructor-aliases with -mno-constructor…
Nov 30 2021, 3:13 PM
modimo closed D114130: [Clang] Add option to disable -mconstructor-aliases with -mno-constructor-aliases.
Nov 30 2021, 3:13 PM · Restricted Project

Nov 18 2021

modimo updated the diff for D114130: [Clang] Add option to disable -mconstructor-aliases with -mno-constructor-aliases.

Condense lines

Nov 18 2021, 1:26 PM · Restricted Project

Nov 17 2021

modimo updated the summary of D114130: [Clang] Add option to disable -mconstructor-aliases with -mno-constructor-aliases.
Nov 17 2021, 5:40 PM · Restricted Project
modimo requested review of D114130: [Clang] Add option to disable -mconstructor-aliases with -mno-constructor-aliases.
Nov 17 2021, 5:32 PM · Restricted Project

Nov 12 2021

modimo added a comment to D113523: Add toggling for -fnew-infallible/-fno-new-infallible.

Gentle ping @bruno

Nov 12 2021, 2:44 PM · Restricted Project

Nov 9 2021

modimo updated the diff for D113523: Add toggling for -fnew-infallible/-fno-new-infallible.

Add driver test

Nov 9 2021, 9:55 PM · Restricted Project
modimo updated the diff for D113523: Add toggling for -fnew-infallible/-fno-new-infallible.

Remove whitespace change

Nov 9 2021, 3:42 PM · Restricted Project
modimo updated the summary of D113523: Add toggling for -fnew-infallible/-fno-new-infallible.
Nov 9 2021, 3:36 PM · Restricted Project
modimo requested review of D113523: Add toggling for -fnew-infallible/-fno-new-infallible.
Nov 9 2021, 3:34 PM · Restricted Project

Oct 29 2021

modimo added inline comments to D112858: [llvm-profgen] Fill zero count for all function ranges.
Oct 29 2021, 6:20 PM · Restricted Project
modimo added inline comments to D112858: [llvm-profgen] Fill zero count for all function ranges.
Oct 29 2021, 5:28 PM · Restricted Project
modimo added a comment to D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

Thanks for the reviews @mtrofin !

Oct 29 2021, 12:34 PM · Restricted Project
modimo committed rG5caad9b5d354: [InlineAdvisor] Add fallback/format switches and negative remark processing to… (authored by modimo).
[InlineAdvisor] Add fallback/format switches and negative remark processing to…
Oct 29 2021, 12:32 PM
modimo closed D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 29 2021, 12:32 PM · Restricted Project
modimo updated the diff for D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

Rebase and add -disable-output consistently to inline-replay.ll

Oct 29 2021, 12:30 PM · Restricted Project
modimo committed rG51ce567b38ec: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect (authored by modimo).
[SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect
Oct 29 2021, 12:05 PM
modimo closed D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 29 2021, 12:05 PM · Restricted Project

Oct 28 2021

modimo added inline comments to D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 28 2021, 4:31 PM · Restricted Project
modimo updated the diff for D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

Move more things to ReplayInlineAdvisor.h and other feedback

Oct 28 2021, 4:31 PM · Restricted Project
modimo added inline comments to D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 28 2021, 4:18 PM · Restricted Project
modimo accepted D112745: Point replay file to non-existent dummy.

Thanks, I'll make sure to avoid pointing at directories to validate these things in the future.

Oct 28 2021, 12:17 PM · Restricted Project

Oct 27 2021

modimo updated the diff for D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

Fix up comment

Oct 27 2021, 10:26 PM · Restricted Project
modimo added inline comments to D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 27 2021, 9:24 PM · Restricted Project
modimo updated the diff for D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

ReplayMode->ReplayFallback, refactoring

Oct 27 2021, 9:24 PM · Restricted Project
modimo retitled D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner from [InlineAdvisor] Add mode/format switches and negative remark processing to Replay Inliner to [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 27 2021, 9:18 PM · Restricted Project
modimo added inline comments to D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 27 2021, 5:22 PM · Restricted Project
modimo updated the diff for D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.

Review feedback

Oct 27 2021, 5:22 PM · Restricted Project

Oct 21 2021

modimo added inline comments to D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 21 2021, 5:16 PM · Restricted Project
modimo updated the diff for D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.

Add comment to SampleProfileLoader::getInlineCandidate. Add inline-topdown-missing.prof file that wasn't part of the diff before.

Oct 21 2021, 5:16 PM · Restricted Project
modimo added a comment to D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.

@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:

Oct 21 2021, 2:53 PM · Restricted Project
modimo updated the diff for D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.

Refactor uses of replay advisor into getExternalInlineAdvisorCost and getExternalInlineAdvisorShouldInline. Move importing code to findExternalInlineCandidate

Oct 21 2021, 2:49 PM · Restricted Project
modimo updated subscribers of D111806: [LICM] Check the number of divergent paths from loop header to target block.

To keep the conversation coherent I'll talk about the same feedback for D87551 here:

Oct 21 2021, 2:23 PM · Restricted Project

Oct 18 2021

modimo added inline comments to D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 18 2021, 7:20 PM · Restricted Project
modimo updated the diff for D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.

Use getCanonicalFnName, add same path to inlineHotFunctionsWithPriority

Oct 18 2021, 7:18 PM · Restricted Project
modimo updated the summary of D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 18 2021, 5:22 PM · Restricted Project
modimo requested review of D112040: [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.
Oct 18 2021, 5:11 PM · Restricted Project
modimo committed rG41f814589f20: [InlineAdvisor][NFC] Fix tests added in D110658 V2 (authored by modimo).
[InlineAdvisor][NFC] Fix tests added in D110658 V2
Oct 18 2021, 3:27 PM
modimo updated the summary of D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 18 2021, 2:34 PM · Restricted Project
modimo updated the summary of D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 18 2021, 2:33 PM · Restricted Project
modimo requested review of D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect.
Oct 18 2021, 2:24 PM · Restricted Project
modimo committed rG2786dc1096a5: [InlineAdvisor][NFC] Fix tests added in D110658 on (authored by modimo).
[InlineAdvisor][NFC] Fix tests added in D110658 on
Oct 18 2021, 2:21 PM
modimo committed rG313c657fcea3: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay… (authored by modimo).
[InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay…
Oct 18 2021, 1:09 PM
modimo closed D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.
Oct 18 2021, 1:09 PM · Restricted Project

Oct 13 2021

modimo updated the diff for D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

Fix up description so to be consistent in that Function scope is default.

Oct 13 2021, 4:56 PM · Restricted Project
modimo retitled D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope from [InlineAdvisor] Add -inline-replay-strict to replay inline decisions only in callers that have remarks to [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.
Oct 13 2021, 4:54 PM · Restricted Project

Oct 12 2021

modimo added a comment to D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

lgtm, thanks.

That being said, negative mode itself is different in that it assumes all other legal sites will be inlined.

Depends on how we define it. This is how I think about it now: in non-strict mode, positive/negative means do or don't do certain inlining, but leave the rest to default heuristic. With strict mode, anything unspecific gets the opposite replay decision (not inline for positive, inline for negative). With that definition, negative is no different in that it can have both strict and non-strict mode.

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.

Oct 12 2021, 6:28 PM · Restricted Project
modimo added a comment to D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

@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?

Oct 12 2021, 5:49 PM · Restricted Project
modimo added a comment to D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

Thanks for working on this. Though perhaps there's some misunderstanding on what "strict" mode means (sorry I should have clarified more beforehand). In the original inline replay patch (https://reviews.llvm.org/D83743), I mentioned strict/positive/negative mode.

The only mode we have before and after this patch is the strict mode. "strict" means existing remarks indicate positive decision, and the rest are all considered negative decision. "positive" means existing remarks indicate positive decision, but the rest is left to other heuristics (or fall back advisors). Accordingly, negative means the input only specifies negative decision.

The change is limiting the scope of replay to function, as opposed to module or whole program. A powerful inline replay framework would support scope={module|function} x mode={strict|positive|negative}.

So I would suggest adding a flag -inline-replay-scope=[function|module] (with a default value), instead of -inline-replay-strict. Later on if we add positive/negative mode, we could add flag -inline-replay-mode=[strict|positive|negative], but for now we don't need it. Thoughts?

I think using scope to capture the functionality here makes sense. For your definition of strict I see it as modifier to positive/negative. A strict+negative replay would not inline the remarks present but will attempt to inline everything else and strict+positive would inline the remarks but not inline anything else. Non-strict+negative would make sure the remarks are not inlined but defer the rest of the decisions to the original advisor and non-strict+positive will inline the remarks but defer the rest to the original advisor.

Strict+negative is useful because one way we can capture replay is by looking at all call-sites that remain in the assembly and eliminate all the rest (i.e. making sure they're inlined) which has the advantage that inlined sites no longer present in the assembly (e.g. simple accessors whose bodies completely get folded) will properly be inlined while in strict+positive mode because the dwarf information is gone no remarks are generated and the original call remains.

Using strict as modifier would make it even more flexible. I didn't think strict+negative would be useful, so didn't give it much thought. Actually it's a bit weird to represent negative decision using the same remark format (... inlined into ...) but relying on flag to tell positive vs negative, perhaps negative-ness can be represented with the remark line using ... not inlined into ... (same as missed remarks). This eliminates the need to tell positive/negative from command line and also enable mixing of positive and negative within a file (for non-strict mode). But we can defer that discussion/decision.

Oct 12 2021, 5:45 PM · Restricted Project
modimo updated the diff for D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

Review feedback. InlineCallersFromRemarks->CallersToReplay. Refactor creating a ReplayInlineAdvisor to use getReplayInlineAdvisor.

Oct 12 2021, 5:45 PM · Restricted Project

Oct 11 2021

modimo added a comment to D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

Thanks for working on this. Though perhaps there's some misunderstanding on what "strict" mode means (sorry I should have clarified more beforehand). In the original inline replay patch (https://reviews.llvm.org/D83743), I mentioned strict/positive/negative mode.

The only mode we have before and after this patch is the strict mode. "strict" means existing remarks indicate positive decision, and the rest are all considered negative decision. "positive" means existing remarks indicate positive decision, but the rest is left to other heuristics (or fall back advisors). Accordingly, negative means the input only specifies negative decision.

The change is limiting the scope of replay to function, as opposed to module or whole program. A powerful inline replay framework would support scope={module|function} x mode={strict|positive|negative}.

So I would suggest adding a flag -inline-replay-scope=[function|module] (with a default value), instead of -inline-replay-strict. Later on if we add positive/negative mode, we could add flag -inline-replay-mode=[strict|positive|negative], but for now we don't need it. Thoughts?

Oct 11 2021, 11:18 PM · Restricted Project
modimo updated the diff for D110658: [InlineAdvisor] Add -inline-replay-scope=<Function|Module> to control replay scope.

Address feedback and change over to replay-scope

Oct 11 2021, 11:17 PM · Restricted Project
modimo committed rGef643617b813: [NFC][LangRef] Update description for FuncFlags (authored by modimo).
[NFC][LangRef] Update description for FuncFlags
Oct 11 2021, 10:04 PM
modimo closed D111600: [NFC][LangRef] Update description for FuncFlags.
Oct 11 2021, 10:04 PM · Restricted Project