Added Kaleidoscope example #19

71 · 2017-10-02T21:40:39Z

Added the Kaleidoscope example to the project. Additionally, write_bitcode_to_file now compiles on Windows (although it will always fail and return false).

Note: Yes, the first three commits have the wrong author, but after an hour of trying to fix this, I just give up on the fame.

71 · 2017-10-02T21:42:03Z

Oh, by the way, this should lead to the update of #2.

codecov · 2017-10-02T21:47:09Z

Codecov Report

Merging #19 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master      #19   +/-   ##
=======================================
  Coverage   46.06%   46.06%           
=======================================
  Files          35       35           
  Lines        3645     3645           
=======================================
  Hits         1679     1679           
  Misses       1966     1966

Impacted Files	Coverage Δ
src/execution_engine.rs	`35.95% <ø> (ø)`	⬆️
src/module.rs	`49.26% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdc1560...92ae1da. Read the comment docs.

71 · 2017-10-03T09:08:31Z

Want me to add tests to Kaleidoscope? In any case, I'll add some improvements, and after this I'm planning to work on the documentation. I also have some remarks on some structures and enums

TheDan64

There's a lot here, so I'm going to do another pass or two in the next couple days. But I did my initial review and it looks great overall.

I'd prefer to avoid transmute like the plague, so please help me understand why they're really needed so we can think about how to replace the ones that aren't.

Also, kaleidoscope.rs is huge! Thank you for all of the time that you've put into this. I really appreciate it. I think we should split it into at least three or four files:

examples/kaleidoscope/lexer.rs
examples/kaleidoscope/parser.rs
examples/kaleidoscope/llvm.rs
and maybe a main/drive type of file, where we can add commandline argument parsing stuff, if needed?

TheDan64 · 2017-10-03T11:39:43Z

examples/kaleidoscope.rs

+    };
+}
+
+pub extern fn putchard(x: f64) -> f64 {


What are these extern functions for? If you need putchar, you should be able to call the llvm putchar function. It's also unclear why you truncate a 64bit float down to an 8 bit int and then back up to a char?

TheDan64 · 2017-10-03T11:41:51Z

examples/kaleidoscope.rs

+            fpm: pass_manager,
+            module: module,
+            function: function,
+            fn_value: unsafe { std::mem::transmute(0usize) },


Transmute should be avoided unless absolutely necessary. It looks to me like this could be replaced with a None of type Option<FunctionValue>?

TheDan64 · 2017-10-03T11:45:54Z

examples/kaleidoscope.rs

+        if is_anonymous {
+            let ee = module.create_jit_execution_engine(0).unwrap();
+
+            // 2017-02-10 <6A> I still can't add my own functions with either add_global_mapping or add_symbol.


Could you explain why add_global_mapping and add_symbol are needed? Also, I don't believe LLVMAddSymbol was introduced until 3.7, and I would like this to eventually work in 3.6 as well.

TheDan64 · 2017-10-03T11:47:58Z

examples/kaleidoscope.rs

+
+/// Represents a primitive syntax token.
+#[derive(Debug, Clone)]
+pub enum Token {


Just a personal preference, but would you mind keeping one enum variant per line and alphabetizing them? I find it's easier to visually find variants when looking at it that way.

TheDan64 · 2017-10-03T11:50:41Z

examples/kaleidoscope.rs

+
+        // Skip whitespaces
+        loop {
+            {


It's hard to tell from just looking at the code, do the two scopes need to be separate? If so, maybe add a comment explaining why.

TheDan64 · 2017-10-03T11:57:10Z

examples/kaleidoscope.rs

+                    let var_val = self.compile_expr(&right)?;
+                    let var = self.variables.get(var_name.as_str()).ok_or("")?;
+
+                    Ok(unsafe { std::mem::transmute(self.builder.build_store(&var, &var_val)) })


What is this transmuting to? IIRC store doesn't return useful data but the instruction. Maybe you want to return the actual value that was stored?

The method returns a FloatValue, but build_store returns a PointerValue. I'm casting the latter to the former here, since the structure is actually a pointer to a float.

TheDan64 · 2017-10-03T11:58:29Z

examples/kaleidoscope.rs

+                    (&else_val, &else_bb)
+                ]);
+
+                Ok(unsafe { std::mem::transmute(phi) })


You should be able to do phi.as_basic_value().into_float_value() or similar in the latest version of Inkwell

TheDan64 · 2017-10-03T12:15:11Z

examples/kaleidoscope.rs

+                let curr_var = self.builder.build_load(&start_alloca, var_name);
+                let next_var = self.builder.build_float_add(&curr_var.as_float_value(), &step, "nextvar");
+
+                self.builder.build_store(&unsafe { std::mem::transmute(next_var) }, &start_alloca);


Why is a float being used as a pointer? Did you mean build_store(&start_alloca, &next_var)?

Good catch, thanks!

71 · 2017-10-03T15:08:08Z

Yeah, in theory transmute should never be used, but I let it up the time to fix some issues I found with Inkwell. Furthermore everything is in a single file, which is, to me, a plus. It is a little less overwhelming to see a single file with 1300 LoC than 5 smaller files. But this is your project, and thus your choice. I'll reply to your other comments as soon as I have the time.

71 · 2017-10-03T18:01:00Z

My last commit should address all your concerns. The only transmute call left is necessary in order to invoke the compiled function.

TheDan64 · 2017-10-03T20:22:03Z

I got to try it out and I like it a lot!

Some thoughts:

> arrow has a space to the left of it that seems out of place
Hitting enter at the prompt panics with index out of bounds, maybe this should just be a no-op that prints a newline and the prompt again?
LLVM IR seems to always output, and 2x when the cli arg is passed in
Even if you don't want to split up the file, it should still go in its own directory, along with a README.md describing the basic syntax (stuff like variable declarations, functions, etc)

TheDan64 · 2017-10-03T20:29:05Z

src/execution_engine.rs

@@ -50,6 +50,12 @@ impl ExecutionEngine {
        }
    }

+    pub fn add_global_mapping(&mut self, global: &AnyValue, addr: *const ()) {


I think it might make sense to remove this for now since it's not used and is likely to change anyway. Long term it may end up looking something like this:

struct MemoryAddress(usize); pub fn add_global_mapping(&(mut?) self, global: &GlobalValue, addr: &MemoryAddress) {}

For example, the execution engine outputs an address which can easily be moved or copied out of the EE's scope, so I think that will end up returning a non-Copy &MemoryAddress so that the underlying value is kept in scope but is still transmutable to a function. Maybe the user will be able to create their own (btw, you can safely cast any pointer to a usize like so: let ptr_val = &a as *const _ as usize; or similar)

What do you think?

TheDan64 · 2017-10-03T20:35:49Z

src/module.rs

        // REVIEW: as_raw_fd docs suggest it only works in *nix
        // Also, should_close should maybe be hardcoded to true?
        unsafe {
            LLVMWriteBitcodeToFD(self.module, file.as_raw_fd(), should_close as i32, unbuffered as i32) == 0
        }
    }

+    #[cfg(windows)]
+    #[allow(unused_variables)]
+    pub fn write_bitcode_to_file(&self, file: &File, should_close: bool, unbuffered: bool) -> bool {


Sweet! Thanks! I hope we'll be able to get this working on windows eventually, and if not we'll probably just remove the method completely from windows.

Getting the file's raw handle might work, but I haven't tried it out.

TheDan64 · 2017-10-03T20:43:10Z

examples/kaleidoscope.rs

+                loop {
+                    let ch = match chars.peek() {
+                        Some(ch) => *ch,
+                        None => { return Ok(Token::EOF); }


Nit, I believe you can just do None => return Ok(Token::EOF),

TheDan64 · 2017-10-03T20:43:34Z

examples/kaleidoscope.rs

+                loop {
+                    let ch = match chars.peek() {
+                        Some(ch) => *ch,
+                        None => { return Ok(Token::EOF); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:43:55Z

examples/kaleidoscope.rs

+
+                let op = match self.curr() {
+                    Op(ch) => ch,
+                    _ => { return Err("Expected operator in custom operator declaration."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:44:12Z

examples/kaleidoscope.rs

+
+                let op = match self.curr() {
+                    Op(ch) => ch,
+                    _ => { return Err("Expected operator in custom operator declaration."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:44:49Z

examples/kaleidoscope.rs

+                (name, true, 0)
+            },
+
+            _ => { return Err("Expected identifier in prototype declaration.") }


No need for brackets

TheDan64 · 2017-10-03T20:45:02Z

examples/kaleidoscope.rs

+
+        match self.curr() {
+            LParen => (),
+            _ => { return Err("Expected '(' character in prototype declaration.") }


No need for brackets

TheDan64 · 2017-10-03T20:45:45Z

examples/kaleidoscope.rs

+        loop {
+            match self.curr() {
+                Ident(name) => args.push(name),
+                _ => { return Err("Expected identifier in parameter declaration.") }


No need for brackets

TheDan64 · 2017-10-03T20:45:53Z

examples/kaleidoscope.rs

+                Comma => {
+                    self.advance();   
+                },
+                _ => { return Err("Expected ',' or ')' character in prototype declaration.") }


No need for brackets

TheDan64 · 2017-10-03T20:46:11Z

examples/kaleidoscope.rs

+    fn parse_paren_expr(&mut self) -> Result<Expr, &'static str> {
+        match self.current()? {
+            LParen => (),
+            _ => { return Err("Expected '(' character at start of parenthesized expression.") }


No need for brackets

TheDan64 · 2017-10-03T20:46:20Z

examples/kaleidoscope.rs

+
+        match self.current()? {
+            RParen => (),
+            _ => { return Err("Expected ')' character at end of parenthesized expression.") }


No need for brackets

TheDan64 · 2017-10-03T20:46:37Z

examples/kaleidoscope.rs

+    fn parse_id_expr(&mut self) -> Result<Expr, &'static str> {
+        let id = match self.curr() {
+            Ident(id) => id,
+            _ => { return Err("Expected identifier."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:46:54Z

examples/kaleidoscope.rs

+                    match self.current()? {
+                        Comma => (),
+                        RParen => break,
+                        _ => { return Err("Expected ',' character in function call."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:47:04Z

examples/kaleidoscope.rs

+                self.advance()?;
+                ch
+            },
+            _ => { return self.parse_primary(); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:47:17Z

examples/kaleidoscope.rs

+
+            let op = match self.curr() {
+                Op(op) => op,
+                _ => { return Err("Invalid operator."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:47:31Z

examples/kaleidoscope.rs

+        // eat 'then' token
+        match self.current() {
+            Ok(Then) => { self.advance()? },
+            _ => { return Err("Expected 'then' keyword."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:47:39Z

examples/kaleidoscope.rs

+        // eat 'else' token
+        match self.current() {
+            Ok(Else) => { self.advance()? },
+            _ => { return Err("Expected 'else' keyword."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:48:03Z

examples/kaleidoscope.rs

+
+        let name = match self.curr() {
+            Ident(n) => n,
+            _ => { return Err("Expected identifier in for loop."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:48:08Z

examples/kaleidoscope.rs

+        // eat '=' token
+        match self.curr() {
+            Op('=') => { self.advance()?; },
+            _ => { return Err("Expected '=' character in for loop."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:48:14Z

examples/kaleidoscope.rs

+        // eat ',' token
+        match self.current()? {
+            Comma => { self.advance()?; },
+            _ => { return Err("Expected ',' character in for loop."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:48:22Z

examples/kaleidoscope.rs

+        // eat 'in' token
+        match self.current()? {
+            In => { self.advance()?; },
+            _ => { return Err("Expected 'in' keyword in for loop."); }


No need for brackets or semicolon

TheDan64 · 2017-10-03T20:49:38Z

Some stylistic comments, but this PR is starting to look pretty good. Did you try running clippy on it by any chance?

71 · 2017-10-03T21:01:27Z

Nope, I'll get to it right away. Also thanks for your stylistic comments, but it wasn't worth the trouble to comment every single mistake... And I just realized I let up something I had used for testing purposes... Oops. Oh, and the space on the left of the > prompt is used to align with the returned =>.

TheDan64 · 2017-10-03T21:35:19Z

No trouble at all. I'll leave it up to you as to whether you want the space or not. Maybe consider something like Python's approach:

> 1 + 1
2

71 · 2017-10-03T21:52:47Z

I instead went with a different approach and prefixed all input with ?>, all debug messages with ->, all errors with !> and all output with =>. I also think everything should be good to go. Some examples have been added, as well as a README.

TheDan64 · 2017-10-04T01:19:49Z

Cool!

root and others added 12 commits September 29, 2017 17:47

Values and types can now be copied and cloned.

c979f42

Documented instruction clone; added instruction copy

dbe5a40

Added tests for copying

26bb26a

Added tests for instruction cloning / copying.

ff031c0

Added Kaleidoscope example; not working yet.

65cd8dc

Chapter 6 of kaleidoscope: DONE

60474f3

Added add_global_mapping function to EE

8190a79

Merge remote-tracking branch 'origin/master' into kaleidoscope

cfa1fc5

Removed unnecessary feature 'box_syntax'

20726e2

Added mut variables to kaleidoscope

90ff2de

Made write_bitcode_to_file compile on Windows

374dd3f

Merged module.rs

245ff82

TheDan64 self-requested a review October 3, 2017 01:06

TheDan64 assigned 71 Oct 3, 2017

TheDan64 requested changes Oct 3, 2017

View reviewed changes

TheDan64 reviewed Oct 3, 2017

View reviewed changes

Fixed bugs, improved overall code

0c87e17

TheDan64 reviewed Oct 3, 2017

View reviewed changes

examples/kaleidoscope.rs Outdated

self.advance()?;

ch

},

_ => { return self.parse_primary(); }

Copy link

Owner

TheDan64 Oct 3, 2017

Choose a reason for hiding this comment

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

No need for brackets or semicolon

TheDan64 reviewed Oct 3, 2017

View reviewed changes

Grégoire Geis added 3 commits October 3, 2017 23:10

Removed currently unused add_global_mapping

044ab99

Removed debug informations

78357e8

Improved readability; fixed some parsing bugs

5441589

Grégoire Geis added 2 commits October 3, 2017 23:49

Updated Cargo.toml for new location of kaleidoscope example

91b24ec

Moved kaleidoscope to its own directory

00f430c

Fixed code example

92ae1da

TheDan64 approved these changes Oct 4, 2017

View reviewed changes

TheDan64 merged commit fd5199a into TheDan64:master Oct 4, 2017

Added Kaleidoscope example #19

Added Kaleidoscope example #19

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!