Repository: weld-project/weld (language: rust)

Issue:
Weld caches the result of UDF calls?
Modifying the Weld program here https://github.com/weld-project/weld/blob/b6ef6748cec3f2740032df164fbeff0aeb0b236a/examples/cpp/udfs_from_library/udfs.cpp#L24 into `|x:i64| cudf[add_five,i64](x) + cudf[add_five,i64](x)` returns the expected result (2*x + 10) but only calls `add_five` once.


Discussion / hints:
Hmm that’s strange, there shouldn’t be any caching there — are you printing
something in the function?

On Fri, May 4, 2018 at 7:30 AM Mihai Varga <notifications@github.com> wrote:

> Modifying the Weld program here
> https://github.com/weld-project/weld/blob/b6ef6748cec3f2740032df164fbeff0aeb0b236a/examples/cpp/udfs_from_library/udfs.cpp#L24
> into |x:i64| cudf[add_five,i64](x) + cudf[add_five,i64](x) returns the
> expected result (2*x + 10) but only calls add_five once.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/weld-project/weld/issues/349>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABTCYyv8YV4YCyeoxKK_7gXPyNnDWwCBks5tvGYMgaJpZM4Tyv1D>
> .
>
-- 
Shoumik

I am printing. I haven't changed the code here: https://github.com/weld-project/weld/blob/b6ef6748cec3f2740032df164fbeff0aeb0b236a/examples/cpp/udfs_from_library/lib.cpp#L9 
I've also set breakpoints in gdb and it only stops once.
I see, I believe this is happening because UDFs are technically not supposed to have side effects, so the optimizer sees a repeated call and assigns it to a temporary variable to perform common subexpression elimination (so I suppose it is some form of caching).

I'll remove this functionality for CUDFs to get around this, but keep in mind that UDFs in theory should not modify global state (e.g., because the for loop can call into them in parallel, etc.)


Fix patch:
diff --git a/weld/llvm.rs b/weld/llvm.rs
index e3e0925d0..c13a331e1 100644
--- a/weld/llvm.rs
+++ b/weld/llvm.rs
@@ -13,7 +13,6 @@ use common::WeldRuntimeErrno;
 use std::io::Write;
 use std::path::PathBuf;
 use std::fs::OpenOptions;
-use std::ascii::AsciiExt;
 
 use super::ast::*;
 use super::ast::Type::*;
@@ -3481,7 +3480,7 @@ impl LlvmGenerator {
                                         for s in b.statements.iter() {
                                             self.gen_statement(s, keyfunc, keyfunc_ctx)?
                                         }
-                                        if (i != end_block_index) {
+                                        if i != end_block_index {
                                             if let Terminator::Branch { ref cond, on_true, on_false} = b.terminator {
                                                 keyfunc_ctx.code.add(format!("; {}", b.terminator));
                                                 if self.trace_run {
diff --git a/weld/parser.rs b/weld/parser.rs
index f1c662d5e..b9d1aeb6c 100644
--- a/weld/parser.rs
+++ b/weld/parser.rs
@@ -112,7 +112,7 @@ impl<'t> Parser<'t> {
             self.position
         };
 
-        for i in (self.position - context_length)..min((self.position + context_length), self.tokens.len()-1) {
+        for i in (self.position - context_length)..min(self.position + context_length, self.tokens.len()-1) {
             let token_str = format!("{}", &self.tokens[i]);
             if i == self.position { 
                 string.push_str(format_color(Color::BoldRed, token_str.as_str()).as_str());
diff --git a/weld/sir/mod.rs b/weld/sir/mod.rs
index e61ae1881..77e5f5d43 100644
--- a/weld/sir/mod.rs
+++ b/weld/sir/mod.rs
@@ -250,9 +250,19 @@ impl StatementTracker {
                             sym_ty: &Type,
                             kind: StatementKind) -> Symbol {
 
+        use sir::StatementKind::CUDF;
+
         let site = ProgramSite(func, block);
         let map = self.generated.entry(site).or_insert(fnv::FnvHashMap::default());
 
+        // CUDFs are the only functions that can have side-effects so we always need to give them
+        // a new name.
+        if let CUDF { .. } = kind {
+            let res_sym = prog.add_local(sym_ty, func);
+            prog.funcs[func].blocks[block].add_statement(Statement::new(Some(res_sym.clone()), kind));
+            return res_sym;
+        }
+
         // Return the symbol to use.
         match map.entry(kind.clone()) {
             Entry::Occupied(ent) => {
@@ -595,7 +605,7 @@ impl fmt::Display for ParallelForIter {
             IterKind::NdIter => "nditer",
             IterKind::RangeIter => "rangeiter",
         };
-        
+
         if self.shape.is_some() {
             /* NdIter. Note: end or stride aren't important here, so skpping those.
              * */
@@ -795,7 +805,7 @@ pub fn ast_to_sir(expr: &TypedExpr, multithreaded: bool) -> WeldResult<SirProgra
         // second call is necessary in the case where there are loops in the call graph, since
         // some parameter dependencies may not have been propagated through back edges
         sir_param_correction(&mut prog)?;
-        Ok((prog))
+        Ok(prog)
     } else {
         weld_err!("Expression passed to ast_to_sir was not a Lambda")
     }
@@ -1293,15 +1303,15 @@ fn gen_expr(expr: &TypedExpr,
                     prog.funcs[body_func]
                         .params
                         .insert(data_res.2.clone(), iter.data.ty.clone());
-                    let start_sym = try!(get_iter_sym(&iter.start, prog, &mut cur_func, &mut cur_block, 
+                    let start_sym = try!(get_iter_sym(&iter.start, prog, &mut cur_func, &mut cur_block,
                                                       tracker, multithreaded, body_func));
-                    let end_sym = try!(get_iter_sym(&iter.end, prog, &mut cur_func, &mut cur_block, 
+                    let end_sym = try!(get_iter_sym(&iter.end, prog, &mut cur_func, &mut cur_block,
                                                     tracker, multithreaded, body_func));
-                    let stride_sym = try!(get_iter_sym(&iter.stride, prog, &mut cur_func, &mut cur_block, 
+                    let stride_sym = try!(get_iter_sym(&iter.stride, prog, &mut cur_func, &mut cur_block,
                                                        tracker, multithreaded, body_func));
-                    let shape_sym = try!(get_iter_sym(&iter.shape, prog, &mut cur_func, &mut cur_block, 
+                    let shape_sym = try!(get_iter_sym(&iter.shape, prog, &mut cur_func, &mut cur_block,
                                                        tracker, multithreaded, body_func));
-                    let strides_sym = try!(get_iter_sym(&iter.strides, prog, &mut cur_func, &mut cur_block, 
+                    let strides_sym = try!(get_iter_sym(&iter.strides, prog, &mut cur_func, &mut cur_block,
                                                         tracker, multithreaded, body_func));
                     pf_iters.push(ParallelForIter {
                                       data: data_res.2,
diff --git a/weld/tokenizer.rs b/weld/tokenizer.rs
index e6148a24c..f017cdd79 100644
--- a/weld/tokenizer.rs
+++ b/weld/tokenizer.rs
@@ -9,7 +9,6 @@
 use std::fmt;
 use std::str::FromStr;
 use std::vec::Vec;
-use std::ascii::AsciiExt;
 
 use regex::Regex;
 
diff --git a/weld/transforms/inliner.rs b/weld/transforms/inliner.rs
index e89d802a6..0ff817546 100644
--- a/weld/transforms/inliner.rs
+++ b/weld/transforms/inliner.rs
@@ -157,9 +157,9 @@ pub fn inline_cast(expr: &mut TypedExpr) {
             if let Literal(ref literal_kind) = child_expr.kind {
                 return match (scalar_kind, literal_kind) {
                     (&F64, &I32Literal(a)) => Some(literal_expr(F64Literal((a as f64).to_bits())).unwrap()),
-                    (&I64, &I32Literal(a)) => Some(literal_expr(I64Literal((a as i64))).unwrap()),
+                    (&I64, &I32Literal(a)) => Some(literal_expr(I64Literal(a as i64)).unwrap()),
                     (&F64, &I64Literal(a)) => Some(literal_expr(F64Literal((a as f64).to_bits())).unwrap()),
-                    (&I64, &I64Literal(a)) => Some(literal_expr(I64Literal((a as i64))).unwrap()),
+                    (&I64, &I64Literal(a)) => Some(literal_expr(I64Literal(a as i64)).unwrap()),
                     _ => None,
                 }
             }


Test patch:
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index 8faace83c..25727a870 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -1489,8 +1489,8 @@ fn simple_for_vecmerger_binops() {
         assert_eq!(unsafe { *result.sum.data.offset(i) },
                    input_vec[i as usize] + input_vec[i as usize] * 7);
         assert_eq!(unsafe { *result.prod.data.offset(i) }, input_vec[i as usize] * (i as i64));
-        assert_eq!(unsafe { *result.min.data.offset(i) }, cmp::min(input_vec[i as usize], (i as i64)));
-        assert_eq!(unsafe { *result.max.data.offset(i) }, cmp::max(input_vec[i as usize], (i as i64)));
+        assert_eq!(unsafe { *result.min.data.offset(i) }, cmp::min(input_vec[i as usize], i as i64));
+        assert_eq!(unsafe { *result.max.data.offset(i) }, cmp::max(input_vec[i as usize], i as i64));
     }
     unsafe { free_value_and_module(ret_value) };
 }