[WIP] log #147
[WIP] log #147
Conversation
tests/integration_tests.rs
Outdated
| @@ -1199,7 +1222,9 @@ fn main() { | |||
| ("iters_for_loop", iters_for_loop), | |||
| ("serial_parlib_test", serial_parlib_test), | |||
| ("iters_outofbounds_error_test", iters_outofbounds_error_test), | |||
| ("outofmemory_error_test", outofmemory_error_test)]; | |||
| ("outofmemory_error_test", outofmemory_error_test), | |||
| ("simple_log", simple_log)]; | |||
erf seems to be giving some linking errors?
There was a problem hiding this comment.
Hey Pari,
This looks good overall, but is this with the refactor @deepakn94 had asked about with the Unary Op? That might be a cleaner way of doing this so we don't have so much repeated code among these operators.
python/weld/weldobject.py
Outdated
| @@ -131,6 +132,8 @@ def toWeldFunc(self): | |||
| return text | |||
|
|
|||
| def evaluate(self, restype, verbose=True, decode=True): | |||
| print("in evaluate!") | |||
tests/integration_tests.rs
Outdated
| let ret_value = compile_and_run(code, conf, &input); | ||
| let data = unsafe { weld_value_data(ret_value) as *const f64 }; | ||
|
|
||
| // fixme: test with a real value |
weld/llvm.rs
Outdated
| @@ -36,6 +36,10 @@ static DICTIONARY_CODE: &'static str = include_str!("resources/dictionary.ll"); | |||
| static DICTMERGER_CODE: &'static str = include_str!("resources/dictmerger.ll"); | |||
| static GROUPMERGER_CODE: &'static str = include_str!("resources/groupbuilder.ll"); | |||
|
|
|||
| //fn test(ctx: &mut FunctionContext) { | |||
weld/type_inference.rs
Outdated
| @@ -92,7 +92,7 @@ fn infer_up(expr: &mut PartialExpr, env: &mut TypeMap) -> WeldResult<bool> { | |||
|
|
|||
| /// Infer the type of expr or its children locally based on what is known about some of them. | |||
| /// Return true if any new expression's type was inferred, or an error if types are inconsistent. | |||
| fn infer_locally(expr: &mut PartialExpr, env: &mut TypeMap) -> WeldResult<bool> { | |||
| fn infer_locally(mut expr: &mut PartialExpr, env: &mut TypeMap) -> WeldResult<bool> { | |||
There was a problem hiding this comment.
Why is the mut necessary here?
python/weld/weldobject.py
Outdated
| @@ -7,6 +7,7 @@ | |||
| import ctypes | |||
|
|
|||
| import os | |||
| import sys | |||
python/weld/weldobject.py
Outdated
| @@ -131,6 +132,7 @@ def toWeldFunc(self): | |||
| return text | |||
|
|
|||
| def evaluate(self, restype, verbose=True, decode=True): | |||
| sys.stdout.flush() | |||
weld/llvm.rs
Outdated
| ) | ||
| } | ||
|
|
||
| fn unary_op(&mut self, ctx: &mut FunctionContext, func: &SirFunction, |
There was a problem hiding this comment.
This is a little weird; maybe just pass the UnaryOpKind into this function and produce the llvm_func here? Otherwise if we add other things which don't have llvm in the function name, it will fail and generate erf
There was a problem hiding this comment.
yes that makes sense, thanks! will restructure this like that. I had decomposed this as a separate function when I wasn't using UnaryOp - but I guess maybe now I should just keep everything in gen_func itself?
weld/llvm.rs
Outdated
| op, | ||
| ref child, | ||
| } => { | ||
| let op_name = try!(llvm_unaryop(op)); |
There was a problem hiding this comment.
See comment above; I think it makes more sense to just merge the functionality of these.
|
You could also just make a single function called unary_op that takes the
Kind as an argument.
…On Fri, Jun 30, 2017 at 4:39 PM Parimarjan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In weld/llvm.rs
<#147 (comment)>:
> @@ -125,78 +125,152 @@ impl LlvmGenerator {
/// Return all the code generated so far.
pub fn result(&mut self) -> String {
- format!("; PRELUDE:\n\n{}\n; BODY:\n\n{}",
- self.prelude_code.result(),
- self.body_code.result())
+ format!(
+ "; PRELUDE:\n\n{}\n; BODY:\n\n{}",
+ self.prelude_code.result(),
+ self.body_code.result()
+ )
+ }
+
+ fn unary_op(&mut self, ctx: &mut FunctionContext, func: &SirFunction,
yes that makes sense, thanks! will restructure this like that. I had
decomposed this as a separate function when I wasn't using UnaryOp - but I
guess maybe now I should just keep everything in gen_func itself?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#147 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABTCYz9s_OXZq2IZXCSOJo_6vbOn8kQkks5sJYdAgaJpZM4Niv6g>
.
|
Got rid of rustfmt changes, but still need to refactor llvm.rs better.
|
@parimarjan This looks good! I think there's a typo in the code that's causing a compile error, can you fix that? If it passes tests after that I think it's good to go :) |
|
@sppalkia Pulled in the changes from master, and fixed the compile error. Passes all tests now! |
|
This looks great! I'll merge it in. |
log implementation.