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

Wrap LLVMBuildICmp #12

Closed
TheOnlySilverClaw opened this issue Jan 7, 2017 · 5 comments
Closed

Wrap LLVMBuildICmp #12

TheOnlySilverClaw opened this issue Jan 7, 2017 · 5 comments

Comments

@TheOnlySilverClaw
Copy link
Contributor

TheOnlySilverClaw commented Jan 7, 2017

First of all, I want to say that I really appreciate this project. ;)

Your readme says that higher-level wrappers are added "as the need arises". So, my meta-issue would be whether you take requests on this and how those should be made.

Currently, I could really use the icmp instruction, since branching is kindof pointless without conditions.
If you do not have time for this, I'll take a look at the lower-level API.

@maleadt
Copy link
Owner

maleadt commented Jan 9, 2017

Your readme says that higher-level wrappers are added "as the need arises". So, my meta-issue would be whether you take requests on this and how those should be made.

Definitely! I just meant that I wouldn't have the time to make LLVM.jl into a feature-complete wrapper, so I focused on getting the infrastructure ready along with some "implementation templates" / stubs.

So if you have the time, maybe you could have a look at wrapping the builder calls you require?
It's really not that complicated:

  • Find the low-level functions you want to wrap, in this case LLVMBuildICmp
  • Implement the high-level wrapper, see eg. how add! is implemented
  • add a test

The LLVMIntPredicate is just an enum value, which you can pass like optlevel in the TargetMachine ctor. I should really figure out a way to safely type those enums...

I should also take the time to document the package's internals, but in this case you only need to know that the underlying LLVM API returns opaque reference pointers (like LLVMValueRef) which LLVM.jl wraps in a more-specifically typed object (defined with @reftypedef) like Instruction. You go from a LLVM API pointer to a specific object by calling a constructor, and access an object its underlying reference pointer by calling ref.

Let me know if you have any questions!

@TheOnlySilverClaw
Copy link
Contributor Author

TheOnlySilverClaw commented Jan 9, 2017

Okay, I'm quite new to the world of Julia and LLVM, but I actually tried doing that. In the simplest form, that would be something like this (?):

icmp!(builder::Builder, op::UInt32, lhs::Value, rhs::Value, name::String="") = 
	Instruction(API.LLVMBuildICmp(ref(builder), op, ref(lhs), ref(rhs), name))

I actually got it working for me, but I have some issues left:

  • Is there any special structure to your tests? I'm not quite sure where to put mine and it seems to fail somehow, even though generating IR works fine for me.
  • As you mentioned, should the type of op be LLVMIntPredicate? Or do you have other ideas for wrapping this? Would it even make sense to create a function for each predicate?

Aside from that, I would like to help wrapping some more stuff, as soon as I have the basics figured out. I spent another hour today to even get the package to build again after replacing it with my forked version to try that out...

@maleadt
Copy link
Owner

maleadt commented Jan 9, 2017

  • Is there any special structure to your tests? I'm not quite sure where to put mine and it seems to fail somehow, even though generating IR works fine for me.

The tests fail now already? If so, that warrants an issue report.
I'd put that icmp test in test/irbuilder.jl, other than that there's not much structure. Just insert an instruction and see whether the module contains it, just like how add! is tested.

  • As you mentioned, should the type of op be LLVMIntPredicate? Or do you have other ideas for wrapping this? Would it even make sense to create a function for each predicate?

I'd typeassert it to ::API.LLVMIntPredicate. Even though that's a typealias of UInt32, it'll make the job easier if we ever migrate to properly-typed enum values.

Aside from that, I would like to help wrapping some more stuff, as soon as I have the basics figured out. I spent another hour today to even get the package to build again after replacing it with my forked version to try that out...

Building can be tricky indeed, I don't think the current solution is solid enough yet -- see JuliaLang/julia#19302. What exactly did you run into?

If you can't figure out something, just open a PR. It's sometimes easier to clone a branch and see what's happening than to figure it all out in the comments.

@TheOnlySilverClaw
Copy link
Contributor Author

Well, the tests failed after I tried to add my instruction between some existing tests, they work fine without those changes. ;)

Building failed until I changed the code to use system-llvm, which is version 3.9.1 for me. Apparently, it wanted to use a 3.7.something-version that comes with Julia? I guess I will encounter that sometime again and could open an issue, but it's basically what you already know.

@maleadt
Copy link
Owner

maleadt commented Jan 9, 2017

Building failed until I changed the code to use system-llvm, which is version 3.9.1 for me. Apparently, it wanted to use a 3.7.something-version that comes with Julia? I guess I will encounter that sometime again and could open an issue, but it's basically what you already know.

Yeah we're only compatible with LLVM 3.9+. Beware that USE_SYSTEM_LLVM is not well-supported, because it results in julia loading two copies of LLVM: one it got bundled with, and one loaded by LLVM.jl. The safest solution is to build Julia master from source (not the binary nightlies, because these don't ship llvm-config, headers, ...), which results in a bundled LLVM 3.9.1 loadable by LLVM.jl.

EDIT: if you have some experience with loading multiple copies of dynamic libraries on multiple platforms, please help me improve USE_SYSTEM_LLVM! Something like a cross-platform dlmopen...

EDIT2: added a note to the README about version compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants