This is an archive of the discontinued LLVM Phabricator instance.

[InlineAdvisor] Restructure advisor plugin unittest cmake
ClosedPublic

Authored by IBricchi on Dec 22 2022, 8:57 AM.

Details

Summary

Move the plugin used in the unittest to test Inline Advisor Plugins
into a separate folder to clean up the cmake file for the analysis
tests.

Diff Detail

Event Timeline

IBricchi created this revision.Dec 22 2022, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 8:57 AM
Herald added a subscriber: mtrofin. · View Herald Transcript
IBricchi updated this revision to Diff 484858.Dec 22 2022, 9:03 AM

Add new line at end of each file

This patch addresses the cmake structure concerns mentioned in https://reviews.llvm.org/D139644

IBricchi published this revision for review.Dec 22 2022, 9:06 AM
IBricchi added reviewers: mtrofin, thakis.
IBricchi added a subscriber: ttheodor.
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 9:06 AM
mtrofin accepted this revision.Dec 22 2022, 1:32 PM

lgtm, but please also wait for @thakis 's feedback.

Should the other case at https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt follow suit with a similar design?

This revision is now accepted and ready to land.Dec 22 2022, 1:32 PM
thakis accepted this revision.Dec 22 2022, 7:01 PM

Thanks, this looks like a big improvement to me :)

llvm/unittests/Analysis/CMakeLists.txt
63

(as mentioned elsewhere, we shouldn't use the _with_input_files variant, but that's unrelated here :) )

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

Username: ibricchi
Email: ibricchi@student.ethz.ch

This revision was automatically updated to reflect the committed changes.

Sorry to raise this issue late (due to the holidays), but it seems this change causes a test failure on AIX

shard JSON output does not exist: /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/unittests/Analysis/./AnalysisTests-LLVM-Unit-26345894-43-81.json

https://lab.llvm.org/buildbot/#/builders/214/builds/5000/steps/6/logs/FAIL__LLVM-Unit__81

Do you have any idea why?

Reverted this patch because it's still failing on the AIX bot.

Hi, sorry for the delay.

I see the error but I'm not sure what it could be, would you be able to tag someone who might know more about why this only fails on AXI @Jake-Egan?

Is there any way of testing changes on buildbot so that I can try and try and correct the behaviour?

It seems this problem is hard to investigate without having access to an AIX machine, so we are looking into it.

Perfect, let me know when something comes up

Jake-Egan added inline comments.Jan 20 2023, 3:48 PM
llvm/unittests/Analysis/CMakeLists.txt
68–72
llvm/unittests/Analysis/InlineAdvisorPlugin/CMakeLists.txt
6–10

The problem is that AnalysisTests is missing -brtl in the link step after this change. If you move this AIX related code back to the original file it will fix the problem.

IBricchi reopened this revision.Jan 30 2023, 2:39 AM
This revision is now accepted and ready to land.Jan 30 2023, 2:39 AM

This should address the -brtl failure

Jake-Egan added inline comments.Feb 1 2023, 11:07 AM
llvm/unittests/Analysis/CMakeLists.txt
68–69

Is the WIN32 check not redundant when we check for AIX on the next line? Other than that you should be good to commit again, thanks!

IBricchi updated this revision to Diff 494056.Feb 1 2023, 1:29 PM

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.

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

ibricchi <ibricchi@student.ethz.ch>

This revision was landed with ongoing or failed builds.Feb 14 2023, 12:33 PM
This revision was automatically updated to reflect the committed changes.