Fixed globalStringPtr to emit a gep (rather than a bitcast), and move…

[jfaure]

…d it to IRBuilder/Instruction.hs

[cocreature]

Looks great, thank you! I left a comment requiring a minor stylistic change. It would be awesome if you could add a test for this to LLVM.Test.IRBuilder. Let me know if you’re having trouble with that and I’ll try to help.

[cocreature]

Please don’t use join here. It might seem like a clever idea right now but it makes the code harder to read and understand. I would just go for [ConstantOperand (C.Int 32 0), ConstantOperand (C.Int 32 0)] or if you really want to avoid the duplication bind it in a let.

[jfaure]

If I'm going to add a test case, I'd like to also add a way to verify the validity of the IR generated by the test cases, hopefully in a more astute way then piping llvm:Pretty to lli, any ideas ?

[cocreature]

Adding a testcase that verifies the validity is slightly tricky as the irbuilder is part of llvm-hs-pure which does deliberately not depend on the LLVM C libraries or tooling. So you’ll have to make a test in llvm-hs. There are tons of tests that check that we can convert from the Haskell AST to the C++ representation and then print the IR so it should be fairly easy to use one of those as an example.

[cocreature]

You seem to have deleted your comment asking whether the llvm-hs-pure test suite can depend on llvm-hs but I’ll respond anyway in case the comment got lost by accident: I’d like to avoid that. Circular dependencies on a package level (even if on a component level there is no cycle) cause problems for some build tools, e.g., nix. Since we control both llvm-hs and llvm-hs-pure I don’t think there is anything to be gained by adding a test suite to llvm-hs-pure that depends on llvm-hs and instead I would suggest that you add the test to the test suite of llvm-hs.

[cocreature]

I’ve merged it without a test for now since the previous behavior was definitely broken and I want to include the fix in the 8.0 release. Thanks again for your contribution!

[jfaure]

Ah yes, sorry about that, had forgotten. I didn't find a testing approach currently in the code that covers this case, and in general there doesn't seem to be any way to validate the IR produced by llvm-hs. It's still all too easy to emit llvm that won't even compile, either at the module level or within a function. I'm not even sure what exactly we should test in this case - whether llc successfully compiles the output ? There is also a tricky subtlety associated with producing binaries, I had no trouble producing object files, but more often than not they wouldn't link.

[cocreature]

No worries, we all get busy. Please feel free to ask for help when you can’t find something.

I'm not even sure what exactly we should test in this case - whether llc successfully compiles the output ?

We have a strCheck function that takes an llvm-hs-pure AST (which could be produced by the IRBuilder API) and a textual representation of the LLVM IR and then converts both to the opposite representation and check that they are equal. That’s quite useful for these kind of test cases (and if it doesn’t quite fit for some reason, it is at least a good starting point).