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

Add support for LLVM 12 #10873

Merged
merged 7 commits into from
Aug 2, 2021
Merged

Conversation

Blacksmoke16
Copy link
Member

Fixes #10434

All credit to @maxfierke

@maxfierke
Copy link
Contributor

Hah was hoping to get back to this at some point but distracted myself with the shiny arm64 stuff. I think I got hung up on something in codegen that was breaking with struct returns, so unless something has changed elsewhere, that will probably still need to be addressed?

@Blacksmoke16
Copy link
Member Author

@maxfierke Haha no problem :) I've been using this patch locally as llvm on arch has updated to 12 and can't say I've encountered any problems. I guess we'll let CI be the judge of it.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Jul 5, 2021

So compiler specs actually do fail when compiled with LLVM 12 :/

Invalid memory access (signal 11) at address 0x8
[0x55d3d5496816] *Exception::CallStack::print_backtrace:(Int32 | Nil) +118
[0x55d3d528a82c] ~procProc(Int32, Pointer(LibC::SiginfoT), Pointer(Void), Nil) +316
[0x7fc916ff2870] ???
[0x7fc918b089b6] _ZN4llvm15ValueEnumerator13EnumerateTypeEPNS_4TypeE +246
[0x7fc918b0b0e3] _ZN4llvm15ValueEnumerator19incorporateFunctionERKNS_8FunctionE +211
[0x7fc918af59cd] _ZN4llvm13BitcodeWriter11writeModuleERKNS_6ModuleEbPKNS_18ModuleSummaryIndexEbPSt5arrayIjLm5EE +4861
[0x7fc918af9491] _ZN4llvm18WriteBitcodeToFileERKNS_6ModuleERNS_11raw_ostreamEbPKNS_18ModuleSummaryIndexEbPSt5arrayIjLm5EE +257
[0x7fc918adf16c] LLVMWriteBitcodeToMemoryBuffer +188
[0x55d3d63fe9aa] *LLVM::Module#write_bitcode_to_memory_buffer:LLVM::MemoryBuffer +10
[0x55d3d64905bc] *Crystal::Compiler::CompilationUnit#compile_to_object:(LLVM::Module | Nil) +284
[0x55d3d6490363] *Crystal::Compiler::CompilationUnit#compile:(LLVM::Module | Nil) +51
[0x55d3d5288216] ~procProc(Nil) +550
[0x55d3d55b1b6e] *Fiber#run:(IO::FileDescriptor | Nil) +174
[0x55d3d52846b6] ~proc2Proc(Fiber, (IO::FileDescriptor | Nil)) +6
[0x0] ???
/usr/bin/ld: _main.o: in function `~procProc(Int32, Int32, LibMylib::Struct)@spec:12':
main_module:(.text+0xe3fd): undefined reference to `*struct.LibMylib::Struct::new:struct.LibMylib::Struct'
/usr/bin/ld: G-lobal.o: in function `*Global::x:Int32':
Global:(.text+0x3): undefined reference to `Global::x'
/usr/bin/ld: G-lobal.o: in function `*Global::x=<Int32>:Int32':
Global:(.text+0x15): undefined reference to `Global::x'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o /tmp/cr-spec-504bb544/spec_helper.cr/crystal-spec-output  -rdynamic /tmp/cr-spec-504bb544/extern/temp_c.o -lpcre -lm -lgc -lpthread -levent  -lrt -ldl`
make: *** [Makefile:89: compiler_spec] Error 1

EDIT: Tracked down this failure to codegens proc that takes and returns an extern struct with sret spec. Based on the changelog saying Added type parameter to the sret attribute to continue work on removing pointer element types., I'd guess we're not providing that type parameter or something 🤷.

However, probably doesn't block merging as it's limited to this one specific use case?

@maxfierke
Copy link
Contributor

maxfierke commented Jul 5, 2021

However, probably doesn't block merging as it's limited to this one specific use case?

I think struct returns are fairly important for some C lib bindings

EDIT: ahh wait, this is only with procs that return C structs? starting to remember where I got hung up on this... deep in LLVM land. was trying to figure out why ValueEnumerator was ending up with a null pointer... somewhere. I think it had something to do with not specifying the size of the sret

@Blacksmoke16
Copy link
Member Author

More so meant it could be fixed in another pr. Of course that should be done before saying llvm 12 is officially supported.

byval supports types back to LLVM 9, so enabled that piece too.
@maxfierke
Copy link
Contributor

Updated w/ the struct return stuff ironed out in 31a8aa2, passes the whole compiler suite now

src/llvm/lib_llvm.cr Outdated Show resolved Hide resolved
Despite the LLVM docs specifying byval having an optional type
back to 9.0, it seems it didn't validate on LLVM 10, so playing safe
and leaving for LLVM 12+ only.
Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

Passes all specs on my machine with LLVM12

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Looks good to go 👍 Thanks @maxfierke @Blacksmoke16 🙏

@straight-shoota straight-shoota added this to the 1.2.0 milestone Jul 29, 2021
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Checking for type will short-circuit the more expensive LLVM::Attribute.requires_type? call.

src/llvm/function.cr Outdated Show resolved Hide resolved
src/llvm/value_methods.cr Outdated Show resolved Hide resolved
…irst

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support LLVM 12
7 participants