M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 5
Code Issues 87 Pull Requests 5 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 +157 -27
ychenfo commented 2022-04-05 18:05:32 +08:00
Collaborator
Copy Link
No description provided.
ychenfo force-pushed bound_check from f1a99e4bfb to 86ce513cb5 2022-04-05 18:21:56 +08:00 Compare
ychenfo commented 2022-04-05 18:22:37 +08:00
Author
Collaborator
Copy Link

force-pushed to rebase on the latest master branch

force-pushed to rebase on the latest master branch
pca006132 reviewed 2022-04-05 19:14:36 +08:00
nac3standalone/demo/src/lists.py
@ -230,3 +230,3 @@
output_int32_list([int32(b) for b in bl5])
bl6 = bl[:]
bl6[3:-5] = [True, False, False]
bl6[3:-5] = [True, False]
pca006132 commented 2022-04-05 19:14:36 +08:00
Collaborator
Copy Link

what is this?

what is this?
ychenfo commented 2022-04-06 00:05:21 +08:00
Author
Collaborator
Copy Link

This is the 'fix broken test' commit. This test should not pass because the length of bl6 is 10, so bl[3:-5] is bl[3,5], which has length 2, so this test is actually extending the list, which is not supported.

Previously this test happened to accidentally pass because we do not have the check on the list slice assignment yet, and the value of the assignment accidentally make the test pass. In fact, just by changing the bl6[3:-5] = [True, False, False] to bl6[3:-5] = [True, False, True] or something else, this test will fail before this PR.

This is the 'fix broken test' commit. This test should not pass because the length of `bl6` is 10, so `bl[3:-5]` is `bl[3,5]`, which has length 2, so this test is actually extending the list, which is not supported. Previously this test happened to accidentally pass because we do not have the check on the list slice assignment yet, and the value of the assignment accidentally make the test pass. In fact, just by changing the `bl6[3:-5] = [True, False, False]` to `bl6[3:-5] = [True, False, True]` or something else, this test will fail before this PR.
pca006132 marked this conversation as resolved
pca006132 reviewed 2022-04-06 11:53:14 +08:00
nac3core/src/codegen/mod.rs
@ -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
Collaborator
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
Author
Collaborator
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
Collaborator
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
Author
Collaborator
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% ) ```
pca006132 approved these changes 2022-04-06 13:36:44 +08:00
sb10q merged commit 86ce513cb5 into master 2022-04-06 17:43:03 +08:00
sb10q deleted branch bound_check 2022-04-06 17:43:03 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
pca006132
Labels
Clear labels   
UROP0622

   
future-extension

   
high-priority

   
low-priority
No Label
UROP0622
future-extension
high-priority
low-priority
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#255
No description provided.
Delete Branch "bound_check"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?