User Details
- User Since
- May 20 2019, 4:16 AM (168 w, 1 d)
Tue, Jul 19
Wed, Jul 13
Tue, Jul 12
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:
Mon, Jul 11
[Pipelines] Fix the Clang :: CodeGenCoroutines/coro-elide.cpp
@aeubanks Thank you for the great explanation.
Jul 7 2022
Jul 6 2022
Jun 30 2022
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).
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.
Jun 29 2022
Jun 28 2022
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.
Jun 27 2022
@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.
Rebase the patch on main, remove all the stuff related to the legacy pass manager.
Jun 25 2022
Jun 24 2022
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?
Jun 23 2022
Jun 21 2022
@MaskRay Thank you for the review. I've landed the patch.
Jun 20 2022
Jun 17 2022
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.
Jun 14 2022
Jun 10 2022
Good job! Thank you for the patch series and the usable tool!
Jun 8 2022
Ping. Does this change make sense at all?
Jun 3 2022
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.
Jun 2 2022
Jun 1 2022
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.
May 31 2022
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.
May 30 2022
Good job! Here is a series of comments, please have a look if you don't mind.
Ping.
May 26 2022
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.
May 25 2022
May 24 2022
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,
May 23 2022
Colleagues the patch is ready for review. Could you have a look again? Thank you.
May 19 2022
Good job! I believe the tool will be very usable. Thank you very much.
Create a DominatorTree for a new function
May 18 2022
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.