sampsyo / bril

an educational compiler intermediate representation
https://capra.cs.cornell.edu/bril/
MIT License
579 stars 238 forks source link

Library-izing `brilift` #312

Closed Alex-Fischman closed 8 months ago

Alex-Fischman commented 8 months ago

This PR:

Note: I don't think it makes sense for the compiler to support the speculate and ssa features, so I marked them as unimplemented!(). In contrast, I think that it does make sense to support char, but I'm not confident in my ability to write C without memory leaks, so I marked that as todo!().

Alex-Fischman commented 8 months ago

I've changed Args to RunArgs and added a CompileArgs struct to improve the API. I've also moved RunArgs into main.rs and made main call into compile.

sampsyo commented 8 months ago

Awesome; looking good! It looks like Clippy has one somewhat weird but OK suggestion: https://github.com/sampsyo/bril/actions/runs/8311293204/job/22745133930?pr=312#step:7:28

Something also seems to be up with the tests… simple stuff like br.bril seems to be hitting the todo!()s? https://github.com/sampsyo/bril/actions/runs/8311293204/job/22745134011?pr=312#step:11:195

I can try to take a closer look, since I don't see an obvious reason why this would be triggering.

Alex-Fischman commented 8 months ago

How do I run the CI?

sampsyo commented 8 months ago

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: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Anyway, looking green from here! Thanks! I'll check things out more closely and then hit the button.

sampsyo commented 8 months ago

OK, we're all set! Thanks again for getting this going!!!

I changed things around just a tiny bit: most prominently, making a jit_run interface in lib.rs that acts as a symmetric pair alongside compile.