ping

Oh I see. I was trying to copy over the changes from how it was done in the thinLTO pipeline, but you're right, I only really want the ModuleInlinerPass. I've updated the patch to reflect that.

format

Glad to hear that! Would you mind pushing the fix? I don't have commit access, thanks

I think the issue was that:

I don't have commit access, could you commit it for me? ibricchi <ibricchi@student.ethz.ch>

Ignore tests on windows due to limited pluign suport

Thanks for the suggestions

Thanks for the review! I updated the patch to reflect the comments.

ping

ping

I wanted to check again if someone could commit this for me since I don't have commit access
ibricchi <ibricchi@student.ethz.ch>

Implement fix for AIX cmake issue from https://reviews.llvm.org/D140559

Implement fix for AIX cmake issue from https://reviews.llvm.org/D140559

format

rebase

I don't have commit access, but if this looks good to you @Jake-Egan would you be able to commit the fix.

I'm not 100% sure on the overlap between the two conditions, I just went off what was there before I changed anything. But I've ammended the code to only check for AIX.

This should address the -brtl failure

Perfect, let me know when something comes up

Thanks for the review @hiraditya I've come to realize this patch might not fit well with the other inline-order options. Instead I think it might make more sense to implement the functionality using custom plugins.

Hi, sorry for the delay.

This patch builds off https://reviews.llvm.org/D139644 to add plugin support for inline order. This should allow for full fine grained control of the inliner using plugins.

In D139644#4015314, @thakis wrote:

In D139644#4003692, @thakis wrote:

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

BTW, was this ever answered?

In D139644#4015049, @tmatheson wrote:

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

I don't have commit access, could one of you two commit this for me?

Add new line at end of files

Move all cmake targets into their own subdirectory

Could one of you two push it for me, I don't have commit access.

I moved the plugins into their own directory similar to https://reviews.llvm.org/D140559.

The reason we went with this general structure for the cmake file is that the other unit tests that implement similar functionality implement it like this:

I don'ţhave commit access to llvm, so if you could commit it for me @akyrtzi that would be great.

I've created a patch which should solve the issue.

I managed to re-create the error and I think it was caused by the order in which things were being built. I added a dependancy for the plugin so that Attributes.inc is ready by the time the plugin comes around to being built.

I tried and I couldn't replicate what you described. Would you be able to say what cmake options you're using so I can try those exactly

My bad, should be fixed now

Updated.
Author: ibricchi
Email: ibricchi@student.ethz.ch

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I must have missed that comment in my previous pass, I've now updated the llvm-config.h.cmake file

I think your suggestions make sense, I've updated the cmake file to address those points.

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

Right I see, well there are two targets, the first is the unit tests which uses all the files except "InlineAdvisorPlugin.cpp", and then there's the target for the inlineAdvisorPlugin which only uses "InlineAdvisorPlugin.cpp", so for the first target we need "InlineAdvisorPlugin.cpp" in the optional source, and for the second target we need everything else.

The reason we need to do this is because otherwise we fail this check: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/LLVMProcessSources.cmake#L105

We will see if @phosek has time, but for context we imitated https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt for the cmake changes.