Add trace column grouping to IR #102

tohrnii · 2022-12-10T11:45:38Z

Closes #85.

grjte

This is just a partial review, since I think there should be some changes based on my comments in #101. I left a few comments inline related to #101 and a few other small things

ir/src/trace_column.rs

ir/src/transition_constraints/graph.rs

ir/src/error.rs

ir/src/boundary_constraints.rs

ir/src/lib.rs

ir/src/symbol_table.rs

Al-Kindi-0

Looks good, thank you! I think the only thing missing is to translate the suggestions in #101 to this PR.

grjte

It looks good to me overall! I'm just wondering about managing the trace columns in a similar way to what we just did in the parser, in which case changes would be needed in a few files (although actually it wouldn't be a major refactor)

ir/src/symbol_table.rs

grjte

Looks good to me, thanks @tohrnii! I've left 2 opinionated non-blocking comments inline.

grjte · 2023-01-03T14:51:15Z

ir/src/trace_columns.rs

-    /// A map of a set of trace columns using the declared identifier as the key and the column
-    /// index as the value.
-    columns: BTreeMap<String, usize>,
+    trace_segment: TraceSegment,
+    size: usize,
+    offset: usize,


really minor nit: it feels to me like the offset should come before the size in all cases (e.g. here, in the new function, etc), since it's describing where in the trace segment the group starts (and then size describes where it ends)

Makes sense. My thinking was that the size is a more local property of the column group and should come before the offset which is meaningful in the context of the whole trace. But this argument probably doesn't make sense since trace_segment is also in the context of the whole trace. Will change it.

grjte · 2023-01-03T15:00:07Z

ir/src/transition_stmts/graph.rs

+                let node_index = self.insert_op(Operation::TraceElement(TraceAccess::new(
+                    trace_segment,
+                    col_idx,
+                    0,
+                )));


nit: Maybe we can use constants CURRENT_ROW and NEXT_ROW for the row offsets of 0 and 1 or at least add comments where these TraceAccess structs are created? I think it would be easier to read what's going on for anyone who is new to the codebase or has stepped away for a week and apparently forgotten everything ;)

Changed it to use CURRENT_ROW and NEXT_ROW. Thanks for the suggestion, I used to get confused with this one as well.

Fumuran

Looks good! Thank you!

Al-Kindi-0

Looks good, thank you!

tohrnii force-pushed the tohrnii-col-grouping-ir branch from a3a0eb8 to 3377252 Compare December 10, 2022 12:09

tohrnii mentioned this pull request Dec 10, 2022

Tracking PR for v0.2 release #79

Merged

14 tasks

tohrnii force-pushed the tohrnii-col-grouping-ir branch 3 times, most recently from cb503bc to 21021f7 Compare December 10, 2022 12:45

tohrnii marked this pull request as ready for review December 10, 2022 12:45

tohrnii requested review from bobbinth, Al-Kindi-0 and Fumuran and removed request for Fumuran, bobbinth and Al-Kindi-0 December 10, 2022 12:45

tohrnii marked this pull request as draft December 10, 2022 12:47

tohrnii force-pushed the tohrnii-col-grouping-ir branch 2 times, most recently from 3539173 to 89a0ce0 Compare December 14, 2022 06:36

tohrnii marked this pull request as ready for review December 14, 2022 06:37

tohrnii requested review from bobbinth, Al-Kindi-0 and Fumuran December 14, 2022 06:37

tohrnii force-pushed the tohrnii-col-grouping-ir branch from 89a0ce0 to 3153e37 Compare December 14, 2022 06:50

grjte requested review from grjte and removed request for bobbinth December 14, 2022 12:07

grjte suggested changes Dec 15, 2022

View reviewed changes

tohrnii force-pushed the tohrnii-col-grouping-parser branch from ae7f6ad to 0bb24c7 Compare December 16, 2022 05:55

Al-Kindi-0 reviewed Dec 16, 2022

View reviewed changes

tohrnii force-pushed the tohrnii-col-grouping-parser branch from 0bb24c7 to a731396 Compare December 17, 2022 08:29

tohrnii force-pushed the tohrnii-col-grouping-ir branch from 3153e37 to c2b50bc Compare December 20, 2022 08:49

tohrnii force-pushed the tohrnii-col-grouping-parser branch 2 times, most recently from 1183f5d to 7b970b5 Compare December 21, 2022 15:32

Base automatically changed from tohrnii-col-grouping-parser to next December 22, 2022 14:22

tohrnii added 4 commits December 22, 2022 17:28

test: add trace col group tests and refactor old tests

5007221

refactor(parser): refactor trace columns AST

c036551

feat(ir): add support for trace col groups to IR

0402835

test: add tests for trace column groups to IR

c13bb22

tohrnii force-pushed the tohrnii-col-grouping-ir branch 2 times, most recently from f3f98ff to a62a07f Compare December 23, 2022 11:13

tohrnii requested review from grjte and Al-Kindi-0 December 23, 2022 11:23

tohrnii force-pushed the tohrnii-col-grouping-ir branch 3 times, most recently from b86b427 to 0a287a7 Compare December 23, 2022 11:42

grjte suggested changes Dec 23, 2022

View reviewed changes

ir/src/symbol_table.rs Outdated Show resolved Hide resolved

tohrnii force-pushed the tohrnii-col-grouping-ir branch from 0a287a7 to c3536ce Compare December 23, 2022 14:37

tohrnii requested a review from grjte December 23, 2022 14:39

tohrnii force-pushed the tohrnii-col-grouping-ir branch 4 times, most recently from cd30aa9 to 6c0bb02 Compare December 23, 2022 16:16

grjte approved these changes Jan 3, 2023

View reviewed changes

tohrnii force-pushed the tohrnii-col-grouping-ir branch from 6c0bb02 to 2814d21 Compare January 5, 2023 16:38

test: add integration test for trace column groups

2fdc6f1

tohrnii force-pushed the tohrnii-col-grouping-ir branch from 2814d21 to 2fdc6f1 Compare January 5, 2023 16:41

Fumuran approved these changes Jan 6, 2023

View reviewed changes

Al-Kindi-0 approved these changes Jan 6, 2023

View reviewed changes

tohrnii merged commit 9ffc6ca into next Jan 6, 2023

tohrnii deleted the tohrnii-col-grouping-ir branch January 6, 2023 08:38

tohrnii mentioned this pull request Jan 6, 2023

Update IR to preserve column groupings for codegen #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trace column grouping to IR #102

Add trace column grouping to IR #102

tohrnii commented Dec 10, 2022 •

edited

Loading

grjte left a comment

Al-Kindi-0 left a comment

grjte left a comment

grjte left a comment

grjte Jan 3, 2023

tohrnii Jan 3, 2023

grjte Jan 3, 2023

tohrnii Jan 5, 2023

Fumuran left a comment

Al-Kindi-0 left a comment

Add trace column grouping to IR #102

Add trace column grouping to IR #102

Conversation

tohrnii commented Dec 10, 2022 • edited Loading

grjte left a comment

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

grjte Jan 3, 2023

Choose a reason for hiding this comment

tohrnii Jan 3, 2023

Choose a reason for hiding this comment

grjte Jan 3, 2023

Choose a reason for hiding this comment

tohrnii Jan 5, 2023

Choose a reason for hiding this comment

Fumuran left a comment

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

tohrnii commented Dec 10, 2022 •

edited

Loading