M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 5
Code Issues 89 Pull Requests 7 Releases Wiki Activity

Add bound check for list and range (#185) #255

Merged
sb10q merged 3 commits from bound_check into master 2022-04-06 17:43:03 +08:00
Conversation 1 Commits 3 Files Changed 6 +16 -12
3 changed files with 16 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 7443c5ea0f - Show all commits

13
nac3core/src/codegen/expr.rs
View File

@ -395,9 +395,9 @@ impl<'ctx, 'a> CodeGenContext<'ctx, 'a> {
self.gen_const(generator, &nac3parser::ast::Constant::Str(s.into()), self.primitives.str)
}
pub fn raise_exn<G: CodeGenerator>(
pub fn raise_exn(
&mut self,
generator: &mut G,
generator: &mut dyn CodeGenerator,
name: &str,
msg: BasicValueEnum<'ctx>,
params: [Option<IntValue<'ctx>>; 3],
@ -434,9 +434,9 @@ impl<'ctx, 'a> CodeGenContext<'ctx, 'a> {
gen_raise(generator, self, Some(&zelf.into()), loc);
}
pub fn make_assert<G: CodeGenerator>(
pub fn make_assert(
&mut self,
generator: &mut G,
generator: &mut dyn CodeGenerator,
cond: IntValue<'ctx>,
err_name: &str,
err_msg: &str,
@ -447,9 +447,9 @@ impl<'ctx, 'a> CodeGenContext<'ctx, 'a> {
self.make_assert_impl(generator, cond, err_name, err_msg, params, loc)
}
pub fn make_assert_impl<G: CodeGenerator>(
pub fn make_assert_impl(
&mut self,
generator: &mut G,
generator: &mut dyn CodeGenerator,
cond: IntValue<'ctx>,
err_name: &str,
err_msg: BasicValueEnum<'ctx>,
@ -969,6 +969,7 @@ pub fn gen_expr<'ctx, 'a, G: CodeGenerator>(
ctx: &mut CodeGenContext<'ctx, 'a>,
expr: &Expr<Option<Type>>,
) -> Result<Option<ValueEnum<'ctx>>, String> {
ctx.current_loc = expr.location;
let int32 = ctx.ctx.i32_type();
let zero = int32.const_int(0, false);
Ok(Some(match &expr.node {

6
nac3core/src/codegen/mod.rs
View File

@ -20,7 +20,7 @@ use inkwell::{
values::{BasicValueEnum, FunctionValue, PhiValue, PointerValue}
};
use itertools::Itertools;
use nac3parser::ast::{Stmt, StrRef};
use nac3parser::ast::{Stmt, StrRef, Location};
use parking_lot::{Condvar, Mutex};
use std::collections::{HashMap, HashSet};
use std::sync::{
@ -77,6 +77,7 @@ pub struct CodeGenContext<'ctx, 'a> {
pub outer_catch_clauses:
Option<(Vec<Option<BasicValueEnum<'ctx>>>, BasicBlock<'ctx>, PhiValue<'ctx>)>,
pub need_sret: bool,
pub current_loc: Location,
pca006132 commented 2022-04-06 11:53:14 +08:00
Review
Copy Link

I wonder if we should add current_loc to CodeGenContext. You can pass the location information when you call functions like list assignment, so this change is really not necessary. The problem with this is that the location information is only updated when we visit Expr nodes, but not updated for other nodes such as statements.

I wonder if we should add `current_loc` to `CodeGenContext`. You can pass the location information when you call functions like list assignment, so this change is really not necessary. The problem with this is that the location information is only updated when we visit `Expr` nodes, but not updated for other nodes such as statements.
ychenfo commented 2022-04-06 13:07:46 +08:00
Review
Copy Link

I wrote it such that it is also updated when visiting statements in gen_stmt.. And I think this along with gen_expr should suffice for updating the location? The location for the current several exception throwed seems fine when I tested them.

Indeed we can pass the location in an additional argument to some functions.. but I just thought that it might be useful to add it here so that later we do not need to bother changing a lot of function signatures again when location information is needed elsewhere.

I also did a profiling on the performance impact of this using a file that is ~10k lines and generated by repeating the mandelbrot.py, and it seems fine?

in master

cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py

 Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs):

          17097.89 msec task-clock:u              #    1.001 CPUs utilized ( +-  1.88% )
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
            657953      page-faults:u             #   38.518 K/sec         ( +-  0.68% )
   <not supported>      cycles:u                                                    
   <not supported>      instructions:u                                              
   <not supported>      branches:u                                                  
   <not supported>      branch-misses:u                                             

            17.083 +- 0.321 seconds time elapsed  ( +-  1.88% )

after this PR

cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py

 Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs):

          18025.91 msec task-clock:u              #    1.037 CPUs utilized  ( +-  1.70% )
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
            650210      page-faults:u             #   37.397 K/sec          ( +-  0.60% )
   <not supported>      cycles:u                                                    
   <not supported>      instructions:u                                              
   <not supported>      branches:u                                                  
   <not supported>      branch-misses:u                                             

            17.389 +- 0.306 seconds time elapsed  ( +-  1.76% )
I wrote it such that it is also updated when visiting statements in `gen_stmt`.. And I think this along with `gen_expr` should suffice for updating the location? The location for the current several exception throwed seems fine when I tested them. Indeed we can pass the location in an additional argument to some functions.. but I just thought that it might be useful to add it here so that later we do not need to bother changing a lot of function signatures again when location information is needed elsewhere. I also did a profiling on the performance impact of this using a file that is ~10k lines and generated by repeating the `mandelbrot.py`, and it seems fine? in master ``` cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs): 17097.89 msec task-clock:u # 1.001 CPUs utilized ( +- 1.88% ) 0 context-switches:u # 0.000 /sec 0 cpu-migrations:u # 0.000 /sec 657953 page-faults:u # 38.518 K/sec ( +- 0.68% ) <not supported> cycles:u <not supported> instructions:u <not supported> branches:u <not supported> branch-misses:u 17.083 +- 0.321 seconds time elapsed ( +- 1.88% ) ``` after this PR ``` cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs): 18025.91 msec task-clock:u # 1.037 CPUs utilized ( +- 1.70% ) 0 context-switches:u # 0.000 /sec 0 cpu-migrations:u # 0.000 /sec 650210 page-faults:u # 37.397 K/sec ( +- 0.60% ) <not supported> cycles:u <not supported> instructions:u <not supported> branches:u <not supported> branch-misses:u 17.389 +- 0.306 seconds time elapsed ( +- 1.76% ) ```
pca006132 commented 2022-04-06 13:36:31 +08:00
Review
Copy Link

It should be fine if updated when visiting statements. I am not worried about performance issue as this is just a simple copy... But I'm wondering why would it take 17.3s to compile a file with 10k lines...

It should be fine if updated when visiting statements. I am not worried about performance issue as this is just a simple copy... But I'm wondering why would it take 17.3s to compile a file with 10k lines...
ychenfo commented 2022-04-06 15:21:36 +08:00
Review
Copy Link

Oh I run this profile under the debug release just to get a brief idea of the performance impact of this PR. Here are the results under the release build:

current master branch:

cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py

 Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs):

          11283.73 msec task-clock:u              #    1.017 CPUs utilized  ( +-  0.27% )
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
            649035      page-faults:u             #   58.476 K/sec          ( +-  0.79% )
   <not supported>      cycles:u                                                    
   <not supported>      instructions:u                                              
   <not supported>      branches:u                                                  
   <not supported>      branch-misses:u                                             

           11.1000 +- 0.0301 seconds time elapsed  ( +-  0.27% )

after this PR

cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py

 Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs):

          11102.92 msec task-clock:u              #    0.992 CPUs utilized  ( +-  0.23% )
                 0      context-switches:u        #    0.000 /sec                   
                 0      cpu-migrations:u          #    0.000 /sec                   
            640474      page-faults:u             #   57.224 K/sec          ( +-  1.02% )
   <not supported>      cycles:u                                                    
   <not supported>      instructions:u                                              
   <not supported>      branches:u                                                  
   <not supported>      branch-misses:u                                             

           11.1944 +- 0.0257 seconds time elapsed  ( +-  0.23% )
Oh I run this profile under the debug release just to get a brief idea of the performance impact of this PR. Here are the results under the release build: current master branch: ``` cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs): 11283.73 msec task-clock:u # 1.017 CPUs utilized ( +- 0.27% ) 0 context-switches:u # 0.000 /sec 0 cpu-migrations:u # 0.000 /sec 649035 page-faults:u # 58.476 K/sec ( +- 0.79% ) <not supported> cycles:u <not supported> instructions:u <not supported> branches:u <not supported> branch-misses:u 11.1000 +- 0.0301 seconds time elapsed ( +- 0.27% ) ``` after this PR ``` cresc@Cresc-Thinkbook:~/code/nac3/work1/nac3/nac3standalone/demo$ perf stat -r 10 ./compile_demo.sh ./src/benchmark.py Performance counter stats for './compile_demo.sh ./src/benchmark.py' (10 runs): 11102.92 msec task-clock:u # 0.992 CPUs utilized ( +- 0.23% ) 0 context-switches:u # 0.000 /sec 0 cpu-migrations:u # 0.000 /sec 640474 page-faults:u # 57.224 K/sec ( +- 1.02% ) <not supported> cycles:u <not supported> instructions:u <not supported> branches:u <not supported> branch-misses:u 11.1944 +- 0.0257 seconds time elapsed ( +- 0.23% ) ```
}
impl<'ctx, 'a> CodeGenContext<'ctx, 'a> {
@ -570,7 +571,8 @@ pub fn gen_func_impl<'ctx, G: CodeGenerator, F: FnOnce(&mut G, &mut CodeGenConte
module,
unifier,
static_value_store,
need_sret: has_sret
need_sret: has_sret,
current_loc: Default::default(),
};
let result = codegen_function(generator, &mut code_gen_context);

9
nac3core/src/codegen/stmt.rs
View File

@ -422,8 +422,8 @@ pub fn final_proxy<'ctx, 'a>(
final_paths.push(block);
}
pub fn get_builtins<'ctx, 'a, G: CodeGenerator>(
generator: &mut G,
pub fn get_builtins<'ctx, 'a>(
generator: &mut dyn CodeGenerator,
ctx: &mut CodeGenContext<'ctx, 'a>,
symbol: &str,
) -> FunctionValue<'ctx> {
@ -519,8 +519,8 @@ pub fn exn_constructor<'ctx, 'a>(
Ok(Some(zelf.into()))
}
pub fn gen_raise<'ctx, 'a, G: CodeGenerator>(
generator: &mut G,
pub fn gen_raise<'ctx, 'a>(
generator: &mut dyn CodeGenerator,
ctx: &mut CodeGenContext<'ctx, 'a>,
exception: Option<&BasicValueEnum<'ctx>>,
loc: Location,
@ -931,6 +931,7 @@ pub fn gen_stmt<'ctx, 'a, G: CodeGenerator>(
ctx: &mut CodeGenContext<'ctx, 'a>,
stmt: &Stmt<Option<Type>>,
) -> Result<(), String> {
ctx.current_loc = stmt.location;
match &stmt.node {
StmtKind::Pass { .. } => {}
StmtKind::Expr { value, .. } => {