Page MenuHomePhabricator

modimo (Di Mo)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Jan 27 2023

modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

That diff also used EHPad as seeds for BFS, I thought we only need LandingPads as seeds? Though I think for simple algorithm, having MIR and IR implementation separated is probably fine. I also don't know where to put a merged version..

if (MBB.isEHPad())
  LandingPads.push_back(&MBB);

LandingPads are the only thing that exists for x86_64 Linux while catchswitch, catchpad, and cleanuppad are all for Windows EH (https://llvm.org/docs/ExceptionHandling.html#new-exception-handling-instructions). Without proper testing on Windows I'd advocate for keeping it to LandingPads only.

For where to put it, right now I'd say there's not a good place for it and we may want to create a new file. Clang has llvm-project/clang/lib/CodeGen/CGException.cpp for all of it's EH support structure but EH on LLVM is very haphazard (I suspect because outside of correctness there was not much optimization thought put into it).

I would support adding a llvm/lib/Analysis/Exceptions.cpp (or whatever name makes sense) like llvm/lib/Analysis/EHPersonalities.cpp exists. Otherwise, if we don't want to add another file I would place it in Function.h/cpp since technically we can consider this calculating a property of the function.

I'm thinking about llvm/include/llvm/Analysis/CFG.h as a place to put the templated code. WDTY?

No, clearly CFG.h operates on a different abstraction level that is unaware of high level concepts like EH. I think we should keep it that way..

I would support adding a llvm/lib/Analysis/Exceptions.cpp (or whatever name makes sense) like llvm/lib/Analysis/EHPersonalities.cpp exists. Otherwise, if we don't want to add another file I would place it in Function.h/cpp since technically we can consider this calculating a property of the function.

I'm not sure this is useful enough to be part of core IR constructs like function/basicblock. Adding its own file seems like a better place than any of the existing ones, though just for one not commonly used function.. That's why I say I don't know :)

Jan 27 2023, 1:09 PM · Restricted Project, Restricted Project
modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

I'm thinking about llvm/include/llvm/Analysis/CFG.h as a place to put the templated code. WDTY?

Jan 27 2023, 12:58 PM · Restricted Project, Restricted Project
modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

That diff also used EHPad as seeds for BFS, I thought we only need LandingPads as seeds? Though I think for simple algorithm, having MIR and IR implementation separated is probably fine. I also don't know where to put a merged version..

if (MBB.isEHPad())
  LandingPads.push_back(&MBB);

LandingPads are the only thing that exists for x86_64 Linux while catchswitch, catchpad, and cleanuppad are all for Windows EH (https://llvm.org/docs/ExceptionHandling.html#new-exception-handling-instructions). Without proper testing on Windows I'd advocate for keeping it to LandingPads only.

Jan 27 2023, 12:41 PM · Restricted Project, Restricted Project
modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

That diff also used EHPad as seeds for BFS, I thought we only need LandingPads as seeds? Though I think for simple algorithm, having MIR and IR implementation separated is probably fine. I also don't know where to put a merged version..

if (MBB.isEHPad())
  LandingPads.push_back(&MBB);
Jan 27 2023, 12:29 PM · Restricted Project, Restricted Project
modimo added inline comments to D142747: [Pseudo Probe] Do not instrument EH blocks..
Jan 27 2023, 12:24 PM · Restricted Project, Restricted Project
modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

Oh I just remembered that code. They're basically the same algorithm. But given that IR and MIR routines are usually implemented separately otherwise templated code would be needed, I'd like to keep as is. WDYT?

Jan 27 2023, 12:19 PM · Restricted Project, Restricted Project
modimo added a comment to D142747: [Pseudo Probe] Do not instrument EH blocks..

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

Jan 27 2023, 11:47 AM · Restricted Project, Restricted Project
modimo added inline comments to D142747: [Pseudo Probe] Do not instrument EH blocks..
Jan 27 2023, 11:45 AM · Restricted Project, Restricted Project

Sep 20 2022

modimo added a comment to D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions.

@iamarchit123 I think the standard advice is to start w/ the llvm-test-suite and then explore other benchmarks as needed. Also, Clang itself is often a very good starting point.

As for profiles, it probably won't be representative, but you could collect the profile using your benchmark and then assess how often the mismatch w/ inlining happens. if you want to do it w/ Clang itself, then a common approach I've heard is to record have Clang build your project and then use ninja trace or equivalent to find the 5-10 TUs w/ the longest compile time. Then stick them in the https://github.com/llvm/llvm-project/tree/main/clang/utils/perf-training directory, which will use them for PGO automatically. If you go that route, you may need to preprocess the source files.

Sep 20 2022, 11:30 AM · Restricted Project, Restricted Project, Restricted Project

Aug 26 2022

modimo added a comment to D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions.

Thanks for taking a look!

Aug 26 2022, 11:47 AM · Restricted Project, Restricted Project, Restricted Project

Aug 17 2022

modimo committed rGe170d955fe57: Split EH code by default (authored by iamarchit123).
Split EH code by default
Aug 17 2022, 12:43 PM · Restricted Project, Restricted Project
modimo closed D131824: Split EH code by default.
Aug 17 2022, 12:42 PM · Restricted Project, Restricted Project

Aug 16 2022

modimo added inline comments to D131824: Split EH code by default.
Aug 16 2022, 4:28 PM · Restricted Project, Restricted Project

Jul 22 2022

modimo committed rG3bb1ce231903: Add a nop instruction if a section starts with landing pad for function splitter (authored by iamarchit123).
Add a nop instruction if a section starts with landing pad for function splitter
Jul 22 2022, 3:20 PM · Restricted Project, Restricted Project
modimo closed D130133: Add a nop instruction if a section starts with landing pad for function splitter.
Jul 22 2022, 3:20 PM · Restricted Project, Restricted Project

Jul 21 2022

modimo accepted D130133: Add a nop instruction if a section starts with landing pad for function splitter.

LGTM aside from minor nit.

Jul 21 2022, 4:20 PM · Restricted Project, Restricted Project

Jun 20 2022

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
Jun 20 2022, 4:54 PM · Restricted Project, Restricted Project
modimo closed D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll.
Jun 20 2022, 4:53 PM · Restricted Project, Restricted Project

Jun 17 2022

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

Jun 15 2022

modimo added inline comments to D127815: [test][AlwaysInline]:Correct comment and file check for always-inline.ll.
Jun 15 2022, 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.
Jun 15 2022, 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, 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