fix: Use places in BB signatures for Hugr generation #342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
- Coverage 92.67% 92.60% -0.07%
==========================================
Files 45 45
Lines 5105 5153 +48
==========================================
+ Hits 4731 4772 +41
- Misses 374 381 +7 ☔ View full report in Codecov by Sentry. |
| x, y = str(p1), str(p2) | ||
| if is_return_var(x) and is_return_var(y): | ||
| return -1 if x < y else 1 | ||
| return -1 if (p1.ty.linear, x) < (p2.ty.linear, y) else 1 |
There was a problem hiding this comment.
The order doesn't seem consistent?
Consider some vars named
x = "%ret_0_linear"
y = "%ret_1_nonlinear"
z = "%zzz_nonlinear"
then x < y, x > z, y < z is a cycle
There was a problem hiding this comment.
Indeed, this %ret business is quite hacky. This currently works since return vars are only compared with each other, hower this changes with @inout.
This special handling of return vars is removed in #340, so I think it's not really worth putting effort into a nice solution here though
| @@ -278,7 +277,7 @@ def leaf_places(place: Place) -> Iterator[Place]: | |||
| yield place | |||
|
|
|||
|
|
|||
| def check_cfg_linearity(cfg: "CheckedCFG") -> None: | |||
| def check_cfg_linearity(cfg: "CheckedCFG[Variable]") -> "CheckedCFG[Place]": | |||
| """Checks whether a CFG satisfies the linearity requirements. | |||
|
|
|||
| Raises a user-error if linearity violations are found. | |||
There was a problem hiding this comment.
Docstring for the new return value?
| def check_cfg( | ||
| cfg: CFG, inputs: VarRow, return_ty: Type, globals: Globals | ||
| ) -> CheckedCFG: | ||
| cfg: CFG, inputs: Row[Variable], return_ty: Type, globals: Globals | ||
| ) -> CheckedCFG[Place]: | ||
| """Type checks a control-flow graph. | ||
|
|
||
| Annotates the basic blocks with input and output type signatures and removes |
There was a problem hiding this comment.
And also mention the places instead of variables in the return type here.
| @@ -117,18 +120,20 @@ def check_cfg( | |||
| } | |||
|
|
|||
| # Finally, run the linearity check | |||
| check_cfg_linearity(checked_cfg) | |||
| from guppylang.checker.linearity_checker import check_cfg_linearity | |||
There was a problem hiding this comment.
Why the local import here, instead of having it at the top?
There was a problem hiding this comment.
Circularity, since linearity_checker.py already depends on cfg_checker.py.
I could move Signature, CheckedBB, and CheckedCFG to checker/core.py to break the cycle if you prefer?
🤖 I have created a release *beep* *boop* --- ## [0.8.0](v0.7.0...v0.8.0) (2024-07-30) ### Features * Parse docstrings for functions ([#334](#334)) ([a7cc97a](a7cc97a)) ### Bug Fixes * Use places in BB signatures for Hugr generation ([#342](#342)) ([48b0e35](48b0e35)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes #337.
The fix for the linearity checker is easy: When checking assignments, only complain if a local unused linear variable is shadowed.
However, correctly lowering the program in #337 to Hugr requires some additional changes to Hugr codegen. Namely, we need to consider if a BB uses a whole struct
sor only some of its fields. In the latter case, we should only feed those used values into the BB. This requires specifying the signature of BBs in the form ofPlaces rather thanVariables.Concretely, this PR does the following:
Signaturegeneric over the representation of program variables so it can capture bothVariableandPlaceCheckedBBandCheckedCFGgenericCheckedCFG[Variable]which then gets turned into aCheckedCFG[Place]during linearity checking