@CarlosAlbertoEnciso No, I haven't. Thank you very much for your amazing work!

@aeubanks Thank you very much for the re-landing.

@fhahn Thank you very much for the suggestion. I've extracted some making sense changes from the patch (using Register instead of unsigned, some C++17-fication, and using references in iterating over the collections in the foreach loops) and committed them in LLVM w/o review because those patches are very small and NFC. This review can be marked as abandoned.

@nlopes Thank you for the information. I've opened the review to get whether this kind of c++17tion makes any sense at all.

@aeubanks Oh, thank you for the comment. I tried to explicitly define EXPENSIVE_CHECK at the beginning of the LazyCallGraph.cpp file and LLVM has successfully been built. So, I've landed the patch.

More structure bindings have been inserted in the foreach loops. Also, some inserts to maps were replaced with the try_emplace operations in order to eliminate redundant copies of pairs (but as I see those pairs are small enough but "no-copy" is better than "copy" even whenever this "no-copy" is for free).

@aeubanks Thank you for the review. This patch doesn't remove any asserts just reformats them. At least it should...

@aeubanks Thank you for the investigation! I believe this patch can be re-landed after D132764 is committed.

@aeubanks Thank you very much for the fix! I tried the cases described in D128830 and to run the new test Analysis/LazyCallGraph/replace-libcall.ll. On my environment, everything works fine, so I accept the revision.

No problem, I've reverted the commit while I need some time to build clang with the reverted commit even to make it clear the commit is guilty.

@mstorsjo Thank you very much for the information. Unfortunately, our tests didn't catch this problem.

Colleagues, thank you for the discussion.

I tried to triage a bit. The test lldb\test\API\functionalities\unused-inlined-parameters\TestUnusedInlinedParameters.py compiles the code in main.c with -O1 and generates the following IR for the @f function:

@Michael137 Thank you very much for the information!

@aeubanks Thank you very much for the benchmark results and patch review. I've landed the patch.

In D128830#3647168, @aeubanks wrote:

... but let me run some internal benchmarks on this patch

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.