Skip to content
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

Fix missing brace that breaks build for LLVM < 10.x #131

Merged
merged 3 commits into from May 27, 2021

Conversation

makise-homura
Copy link
Contributor

Despite of the fact LLVM 9.x is unsupported anymore by llvm-cbe, it still probably works in most cases, if this brace is fixed.

I guess if there's a decision to forbid builds for any LLVM < 10.x, an #error should be there instead of such mistake. Still llvm-cbe seems to be working for me with LLVM 9.0.1, so I see no reason to put #error there instead of this fix.

Should fix issues #130 and #119.

Despite of the fact LLVM 9.x is unsupported anymore by llvm-cbe,
it still probably works in most cases, if this brace is fixed.
@@ -398,6 +398,7 @@ static int compileModule(char **argv, LLVMContext &Context) {
if (RelaxAll) {
if (FileType != CodeGenFileType::CGFT_ObjectFile)
#else
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may fix the build, but it's not checking for RelaxAll any more, which means it will be warning about -mc-relax-all regardless of whether it is set?

Can you see if it compiles if you have the same if (RelaxAll) { for LLVM 9 and below? Alternatively can you make it skip this warning entirely?

I don't really know what this code is doing though. I am surprised object files are mentioned here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed that warning, but thought it was something on my side, not LLVM and/or llvm-cbe.
I've checked LLVM-6, 7, 8, 9, 10, and 11 (every single version that Ubuntu 20.04 has), and figured out that RelaxAll is supported in 8 and 9 too, and using LLVM 6 or 7 does not allow llvm-cbe to build successfully, so I added a check for LLVM < 8 into CMakeLists.txt too, and used RelaxAll for every other version of LLVM < 10.

Checked unittests/CWriterTest and test/selectionsort, everything is performing normally on LLVM 8 to 11.

Also tried to build lzlib and plzip using llvm-cbe, lzlib builds and works perfectly (with any version of LLVM from 8 to 11), but I have no success on plzip: sometimes llvm-cbe just crashes, sometimes it produces types that have not been declared at all or variables that declared after being used. I'm gonna file a couple issues about that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me.

@vtjnash vtjnash merged commit ea8419b into JuliaHubOSS:master May 27, 2021
@makise-homura
Copy link
Contributor Author

Thanks!

@makise-homura makise-homura deleted the llvm9-fix branch May 27, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants