Return the PostOrderFunctionAttrsPass pass back on its original place in the pipeline.

@aeubanks Hmm, if I correctly get your comment, I should revert this patch to the state before the proposed solution with moving the PostOrderFunctionAttrsPass at the end of the buildInlinerPipeline function regardless of the readonly instead of readnone regression. Personally along with your concern about compilation time, I have a concern about some changing in coroutines compilation, the Clang :: CodeGenCoroutines/coro-elide.cpp test demonstrates them:

[Pipelines] Fix the Clang :: CodeGenCoroutines/coro-elide.cpp

@aeubanks Thank you for the great explanation.

Fix Clang :: CodeGen/thinlto-distributed-newpm.ll

Try to remove the DAE from the original point. Also, apply the suggestion from @nikic - make the DAE a part of the normal module optimization pipeline (post module simplification).

In D128830#3622467, @fhahn wrote:

Do we need to retain the run of DeadArgumentEliminationPass in the original position or is a single run at the new position sufficient?

Actualize the new PM tests.

A number of tests failed, I'm going to fix them ASAP.

Add a PhaseOrdering test to show how the patch affects dead argument elimination after argument promotion.

In D128830#3621522, @nikic wrote:

There should be a PhaseOrdering test that shows that the argument is not removed in the current opimization pipeline.

CGSCCPassManager and LazyCallGraph are included in the class' header. About Module I'm not sure, CLion marks this header as unused, I also see no Module in the code.

Remove #include of unused headers.

Eliminate all the getters as well as the ReplaceCallSite parameter.

Good catch, thank you. I think the ReplicaCallSite parameter can be removed too because we pass only None for it.

@nikic Thank you for accepting. I've got the commit access and able to land patches. I'm going to land the patch in a day or two.

Apply the suggestion about doing a LI->setOperand(0, GetAlloca(Ptr)) and not really creating a new instruction and RAUW.

@nikic thank you for landing D128536. The patch has been rebased.

Rebase the patch on main, remove all the stuff related to the legacy pass manager.

Fetch AssumptionCache for the new function rather than the old one.

@nikic I believe this is a very good idea just to drop the legacy PM support in ArgPromotion. Thank you for the patch, once your patch has been landed, I'll just leave the single DTGetter.

Implement the proposed solution with DTGetter to reuse DominatorTrees from the pass manager. This works fine on the new PM but not on the legacy one. As a workaround, a new DominatorTree is created for every new generated function whenever the Argument Promotion Pass is used through the legacy pass manager.

Apply suggestions from the code review.

Address almost the all reviewer's comments.

@nikic Thank you very much for the review and comments, I've addressed almost all of them and asked a pair of questions about alignment check for loads in the HandleEndUser lambda. Could you have a look and give your answers?

@nikic @aeubanks Colleagues, ping, could you have a look at the changes?

@MaskRay Thank you for the review. I've landed the patch.

Ongoing changes LGTM but I would like you to wait for another approve too. @CarlosAlbertoEnciso please upload the final revision.

@CarlosAlbertoEnciso You are welcome! I'll be glad if my modest efforts help you to land this amazing tool.

Good job! Thank you for the patch series and the usable tool!

Ping. Does this change make sense at all?

Here are a series of comments to the new patch. The most comments are just nits (replacing map#insert with map#emplace but sometimes a piecewise construct could make sense however; and manually constructing by [const] Map::value_type & references to a map entry in foreach loops with the already defined in the std::map class template reference and const_reference aliases. Also a couple of possible memory leaks and bugs related to handling Map.end() iterator have been found, have a look, please.

Unfortunately, it looks like @nikic and @aeubanks have no time to review the patch (or maybe I'm getting something wrong and I should have added the nicks via @ in my comments during ping request sending, I'm sorry if you, colleagues, just didn't catch my requests). @chandlerc Could you as a code owner and one of the pass's author have a look at the patch? @jdoerfert @eli.friedman Or you? Thanks a lot in advance.

Good job! I have a few most stylistic only comments here. What I would like to discuss with the other reviewers: should an assert be used in every function that accepts an argument by pointer and dereferences the pointer or calls functions on it (so, dereferences it too)? My personal opinion: yes but there can be other opinions too, so I do not insist.

Rename LLVM_LINKER_WORKS to LLVM_LINKER_SKIP_TEST.

@thieta A problem occurs when I try to build LLVM with runtime components for RISCV, for example, and in fact, CMake cross-compiles the components in this case. So, If I set -DLLVM_ENABLE_LLD=ON, the following errors are issued due to LLVM's attempt to check the linker:

Colleagues, I propose this change to make the runtime components buildable with a "not-ready yet" lld linker and libraries. One disadvantage I see, when the LLVM_USE_LINKER is not set, the CMake issues warnings about the unused variable LLVM_LINKER_WORKS.

Some my comments repeat comments to previous patches, sorry if it looks noisy. Could you check the {A, B} replacement of the std::make_pair(A, B)? If it works, there is a couple of places where std::make_pair is used and they can be replaced with {A, B} too.

Good job! Here is a series of comments, please have a look if you don't mind.

Ping.

Good job! Here is a series of comments, please have a look.

@Ericson2314 Thank you for the comment. I'm going to land this small patch in a day if there will be no objections.

Fix a typo in the comment to the not used anymore isDenselyPacked static member function.

There a lot of small nit comments and a few questions. These questions are by no means a criticism of your code or design decisions, they are here just to let me understand more about using classes in LLVM Support. Thank you in advance for the answers,

Colleagues the patch is ready for review. Could you have a look again? Thank you.

Good job! I believe the tool will be very usable. Thank you very much.

Create a DominatorTree for a new function

Unfortunately, a CallGraphSCCPass pass (the legacy ArgPromotion pass is a child of this class) cannot use a function analysis pass, DominatorTreeWrapperPass for example. It is interesting, the compiled with the new version of the ArgumentPromotion pass opt tool crashes by this reason only during a run of the Polly tests (you can see the crash in the pre-merge checks on Windows).

Implementation for an Interval Tree (light tree data structure to hold intervals).

(in the patch description), I believe there should be a description for the second patch in the series, not the first.

Reformat the code to let the git-clang-format check pass.

I'm not familiar with the discussion of the class design. From the architecture point of view, the class can be divided into a builder that accumulates the intervals and build the tree and an immutable tree class that contains the query methods only. This design can be an overengineering but on the other hand the implemented algorithm (in the find_iterator for example) is not short and it deserves its own class.