-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapt to LLVM 16 #51591
Adapt to LLVM 16 #51591
Conversation
One general question, why not just skip LLVM 16 and go straight to LLVM 17? |
"just" and "straight" are easier said than done: we started the effort to build llvm 16 5 months ago, and that's not even fully done yet. |
Are you volunteering to do that work? LLVM updates are tedious, fraught with risk and while it might be nice to skip an entire version you end up with more work and less ability to find out when something broke, because you can't bisect the middle. If anyone wants to get involve getting the LLVM 16 build into Yggdrasil and looking at LLVM 17 in Yggdrasil would be a major contribution. |
Okay two remaining source files...
The pipeline failures are:
Which makes me think that we are missing one of @pchintalapudi patches from LLVM 15. |
Okay @pchintalapudi this is probably ready for a first review. definitly didn't do the memory attribute transition correctly since:
|
with
|
@@ -98,7 +102,9 @@ struct OptimizationOptions { | |||
|
|||
struct NewPM { | |||
std::unique_ptr<TargetMachine> TM; | |||
#if JL_LLVM_VERSION < 160000 | |||
StandardInstrumentations SI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing all of StandardInstrumentations is not an option since we rely on print-before and print-after for testing, if upstream won't go back and remove the LLVMContext argument in the constructor then we need to create our own standardinstrumentations class that behaves the way we want it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://reviews.llvm.org/D137149 we can either try to copy the code and maintain our own copies of it, or move this state hidden behind the same ThreadSafeContext as controls the LLVMContext. I think I prefer the latter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean creating a pass pipeline per module, certainly doable but we should measure the cost that has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just per context
@@ -351,7 +350,12 @@ static void buildEarlySimplificationPipeline(ModulePassManager &MPM, PassBuilder | |||
FPM.addPass(DCEPass()); | |||
FPM.addPass(SimplifyCFGPass(basicSimplifyCFGOptions())); | |||
if (O.getSpeedupLevel() >= 1) { | |||
#if JL_LLVM_VERSION >= 160000 | |||
// TODO check the LLVM 15 default. | |||
FPM.addPass(SROAPass(SROAOptions::PreserveCFG)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current default is SROAOptions::ModifyCFG, the old default was PreserveCFG
ModifyCFG was added in llvm/llvm-project@4f7e5d2#diff-2b09f41dec5c9d1fbe844989c38f6bb7db588799b97b4bed7ad173ce7f348280R1119 for llvm-16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging that up. So we should use PreserveCFG late in the pipeline and ModifyCFG early on.
@@ -98,7 +102,9 @@ struct OptimizationOptions { | |||
|
|||
struct NewPM { | |||
std::unique_ptr<TargetMachine> TM; | |||
#if JL_LLVM_VERSION < 160000 | |||
StandardInstrumentations SI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://reviews.llvm.org/D137149 we can either try to copy the code and maintain our own copies of it, or move this state hidden behind the same ThreadSafeContext as controls the LLVMContext. I think I prefer the latter
This should be NFC for now so let's land this. |
Co-authored-by: Gabriel Baraldi <[email protected]> Co-authored-by: Prem Chintalapudi <[email protected]> Co-authored-by: Jameson Nash <[email protected]>
Build is failing |
Stupid rebase mistakes. |
I love C++ so much...
starts the slog towards upgrading us to LLVM 16 and C++17.