Library-izing brilift
#312
Conversation
| @@ -31,7 +31,7 @@ itoa = "1.0" | |||
| [dependencies.bril-rs] | |||
| version = "0.1.0" | |||
| path = "../bril-rs" | |||
| features = ["ssa", "memory", "float", "speculate", "char"] | |||
| features = ["memory", "float", "ssa", "speculate", "position", "import", "char"] | |||
There was a problem hiding this comment.
I was surprised that these features weren't added since brilirs supports most of these. I realized that they probably get pulled in during feature resolution with bril2json which does list these features.
|
I've changed |
|
Awesome; looking good! It looks like Clippy has one somewhat weird but OK suggestion: Something also seems to be up with the tests… simple stuff like I can try to take a closer look, since I don't see an obvious reason why this would be triggering. |
There was a problem hiding this comment.
Looking into it a little closer, it seems like compilation crashes when trying to add the runtime functions! I've marked the place where this happens. It seems like maybe we should add a _bril_print_char function, which shouldn't be too complex? Or, we could just remove PrintChar from the relevant enum.
brilift/src/translator.rs
Outdated
| Self::PrintInt => "_bril_print_int", | ||
| Self::PrintBool => "_bril_print_bool", | ||
| Self::PrintFloat => "_bril_print_float", | ||
| Self::PrintChar => todo!(), |
There was a problem hiding this comment.
I think this is the trouble with the tests! Probably the easiest thing here is just to implement the runtime function, which should be straightforward.
There was a problem hiding this comment.
I didn't do this because I don't know how to print Unicode characters from printf.
|
How do I run the CI? |
|
Unfortunately, the CI has to be manually triggered by a maintainer when the PR comes from a first-time contributor. It's a really annoying feature in GitHub that is sadly necessary to prevent widespread crypto scams: Anyway, looking green from here! Thanks! I'll check things out more closely and then hit the button. |
The `CompileArgs` struct wasn't very helpful, so I made these actual arguments to the function. The `OptLevel` enum was also sort of in the wrong place: this is about command-line parsing rather than being about an actual useful interface (since we will pass this as a string to Cranelift anyway).
For symmetry with the `compile` interface.
|
OK, we're all set! Thanks again for getting this going!!! I changed things around just a tiny bit: most prominently, making a |
This PR:
main.rsto new filetranslator.rslib.rsbril-rsfeature set to bothbrilirsandbriliftto allow users to depend on bothtranslator.rsNote: I don't think it makes sense for the compiler to support the
speculateandssafeatures, so I marked them asunimplemented!(). In contrast, I think that it does make sense to supportchar, but I'm not confident in my ability to write C without memory leaks, so I marked that astodo!().