In D142747#4087141, @wenlei wrote:

In D142747#4087114, @hoy wrote:

In D142747#4087112, @modimo wrote:

In D142747#4087088, @modimo wrote:

In D142747#4087069, @wenlei wrote:

In D142747#4086978, @modimo wrote:

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

In D142747#4087114, @hoy wrote:

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

In D142747#4087088, @modimo wrote:

In D142747#4087069, @wenlei wrote:

In D142747#4086978, @modimo wrote:

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.

In D142747#4087069, @wenlei wrote:

In D142747#4086978, @modimo wrote:

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);

In D142747#4087043, @hoy wrote:

In D142747#4086978, @modimo wrote:

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?

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

In D132186#3802989, @paulkirth wrote:

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

Thanks for taking a look!

LGTM aside from minor nit.

In D125495#3510338, @mingmingl wrote:

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.

In D124563#3478978, @aaronpuchert wrote:

In D124563#3478968, @modimo wrote:

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.

In D124563#3478951, @aaronpuchert wrote:

In D124563#3478937, @modimo wrote:

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...

In D124563#3478937, @modimo wrote:

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?

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?

In D124563#3478915, @aaronpuchert wrote:

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

In D124563#3478781, @aaronpuchert wrote:

In D124563#3478653, @modimo wrote:

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.

In D124563#3478627, @smeenai wrote:

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.

In D124563#3478561, @smeenai wrote:

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.)

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

In D119506#3374156, @int3 wrote:

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.

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();
}

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()));

LGTM

LGTM

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.

In D116243#3210118, @aganea wrote:

In D116243#3210116, @modimo wrote:

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

In D116243#3209390, @aganea wrote:

LGTM, thanks!

Remove variable backend version check

Formatting

Condense lines

Gentle ping @bruno

Add driver test

Remove whitespace change

Thanks for the reviews @mtrofin !

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

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.

Fix up comment

ReplayMode->ReplayFallback, refactoring

Review feedback

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:

Use getCanonicalFnName, add same path to inlineHotFunctionsWithPriority