feat!: Monomorphise everything during type checking #1441
Conversation
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
Bencher Report
| state = get_tracing_state() | ||
| # Only capture errors that are tagged with the span of the current | ||
| # tracing node | ||
| if err.error.span is None or to_span(state.node) == to_span(err.error.span): |
There was a problem hiding this comment.
This is slightly annoying:
Before this PR, we ensured that all functions called by comptime functions are checked before we enter capture_guppy_errors. Thus, errors in regular guppy functions would be reported using the normal mechanism.
Now, we can only check those called functions while we're tracing (since we only know the monomorphic instantiation at that point) so their errors would be intercepted here. This guard is a hacky solution to avoid this.
The proper solution would be to do comptime tracing during checking, instead of during Hugr construction, but that's a bigger refactor
There was a problem hiding this comment.
First thing to note is that the method doc is wrong - it mentions "GuppyComptimeException" and such does not exist!
IIUC the issue here is to distinguish between errors checking the guppy generated directly by the comptime block, and those in newly-created instantations of non-comptime methods?
Are you sure that the refactor to do comptime tracing during checking, isn't something we should do first? I think it might help with a few other questions/concerns here and I suspect it'll be a requirement for signature metaprogramming (where, in some sense, many more functions are processed in a way that's like tracing - perhaps all, perhaps not).
Failing that, is there an intermediate solution - can you make guppy-comptime enqueue the instantiations that it needs, and then do "another round" of checking (emptying the queue) after comptime?
There was a problem hiding this comment.
First thing to note is that the method doc is wrong - it mentions "GuppyComptimeException" and such does not exist!
Fixed!
IIUC the issue here is to distinguish between errors checking the guppy generated directly by the comptime block, and those in newly-created instantations of non-comptime methods?
Yes, exactly
Are you sure that the refactor to do comptime tracing during checking, isn't something we should do first?
I think the priority is to get this monomorphisation change in first since it's more important for other features. But I would like to clean up tracing soon!
Failing that, is there an intermediate solution - can you make guppy-comptime enqueue the instantiations that it needs, and then do "another round" of checking (emptying the queue) after comptime?
That's hard since we're already in the process of building the graph while tracing. So we would need to leave some holes in the graph and fill them in later. Not sure if that's worth if we're going to change the way tracing works anyways...
There was a problem hiding this comment.
Can we handle the latter by doing some checking during compilation? Factor compile into a call to check (that empties the to-check worklist) and then does compilation, and if we enqueue more stuff to check then call the same check method again? I mean, it only works if we have a good idea when we might need to do more checking, but "when compiling tracing" seems to get us a fair way towards knowing that (and it may be ok just to empty the to-check worklist every time we push anything onto it).
The above might not be that big a change - indeed this comment suggests we have some interleaving between checking/compile already and maybe that could be enough (/nearly)?
But failing that, well ok, maybe we need to just live with the hack until we can get onto the tracing-during-checking thing. In that case let's just put some comment here to that effect.
Oh, and is there an issue for the comptime refactor? Sounds good, but I haven't entirely understood how this will work yet - will the "new" tracing build ParsedFunctionDef/CheckedFunctionDefs etc. containing newly built python AST?
There was a problem hiding this comment.
I found an alternative way to get rid of the hack: be more conservative about which places are annotated as capture_guppy_errors. See 00202e9
There was a problem hiding this comment.
Oh, and is there an issue for the comptime refactor?
Created issue #1592
Sounds good, but I haven't entirely understood how this will work yet - will the "new" tracing build ParsedFunctionDef/CheckedFunctionDefs etc. containing newly built python AST?
Not sure yet - building checked AST would be the obvious way to go imo, but maybe there are alternatives
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1441 +/- ##
==========================================
- Coverage 93.55% 93.50% -0.05%
==========================================
Files 130 130
Lines 12436 12397 -39
==========================================
- Hits 11634 11592 -42
- Misses 802 805 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Note: Parameter `tag` was instantiated to `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaa` | ||
|
|
There was a problem hiding this comment.
Shame that we also no longer have this
Maybe, we want this as a more general hint to tell us what parameters were instantiated to?
There was a problem hiding this comment.
Yes, can we not ("just"!!) include the current monomorphization as part of the context of the error? (I guess it would be potentially a chain of instantiations, which might be hard to identify given the worklist, but even just the nearest-enclosing instantiation would be something)
There was a problem hiding this comment.
I'm a bit reluctant to add the monomorphization to every error. Ideally, we'd be able to identify for which errors this context might be useful.
I'm imagining an annotation on types and consts that remembers if they arose from a monomorphic instantiation. Then, the errors could check if they refer to such types and print additional context.
But personally, I'd prefer to leave this as a follow up, what do you think?
There was a problem hiding this comment.
I think if we can improve the checking of the uninstantiated decl, then the remaining errors (that are only caught when instantiated) should all have the monomorphization in the error message....because they do not occur without it
There was a problem hiding this comment.
Good point, this seems like a good solution! Unfortunately, this won't help in this case since main is not generic and state_result is checked in place instead of being added to the queue...
In the future we could define state_result like this though:
def state_result(tag: str @ comptime, ...):
assert comptime(len(tag) < 256), "Tag is too long"
_hugr_state_result(...)Comptime assertions like this would be a nice feature I think
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Thanks Mark! A lot to think about here... a few more bits you can cleanup/remove, some thoughts on typechecking, some possible further refactors.
I see that uninstantiated functions do not produce errors (even if they contain e.g. undeclared variables), simply because uncalled functions do not...that does feel a bit sneaky/surprising (I mean it's kinda like python "errors delayed until runtime" but we would often use mypy to detect them ahead of time and guppy takes the role of mypy...)
OTOH it's good to see that a function that's invalid for all type arguments, produces the error only for one set of type arguments....although I think that is just because we only issue one error before we stop....
However this does not produce an error:
T = guppy.type_var("T")
@guppy
def baz(x: T, y: T) -> int:
return x + y
@guppy
def main(x: int, y: int) -> None:
baz(x, y)
main.compile_function()
see comment on engine.py but I'd be in favour of doing that even for unreached functions, really!
| inputs: Row[Variable], | ||
| return_ty: Type, | ||
| generic_params: dict[str, Parameter], | ||
| generic_inst: dict[str, Argument], |
There was a problem hiding this comment.
Super-nit but I preferred generic_args - clearer that it's just the arguments, whereas an "instantiation" might be just that, or (say) both the arguments and the (parametric/polymorphic) thing being instantiated
| f_locals: dict[str, Any] | ||
| f_globals: dict[str, Any] | ||
| f_builtins: dict[str, Any] | ||
| def_id: DefId | None |
There was a problem hiding this comment.
I think we could clarify the doc comment: "Wrapper around one Definition in the DEF_STORE" or something like that. (Can it really be "in a stack frame"? In a function scope, perhaps - I would have thought that stack frames were dynamic, runtime, objects?)
| def check_valid_entry_point(defn: ParsedDef) -> MonoArgs: | ||
| """Checks if the given definition is a valid compilation entry-point. | ||
|
|
||
| In particular, ensures that the definition doesn't depend on generic parameters and |
There was a problem hiding this comment.
super-nit: the "and returns...." should be in the first sentence....or, and perhaps better, the () could just go in the caller
| state = get_tracing_state() | ||
| # Only capture errors that are tagged with the span of the current | ||
| # tracing node | ||
| if err.error.span is None or to_span(state.node) == to_span(err.error.span): |
There was a problem hiding this comment.
First thing to note is that the method doc is wrong - it mentions "GuppyComptimeException" and such does not exist!
IIUC the issue here is to distinguish between errors checking the guppy generated directly by the comptime block, and those in newly-created instantations of non-comptime methods?
Are you sure that the refactor to do comptime tracing during checking, isn't something we should do first? I think it might help with a few other questions/concerns here and I suspect it'll be a requirement for signature metaprogramming (where, in some sense, many more functions are processed in a way that's like tracing - perhaps all, perhaps not).
Failing that, is there an intermediate solution - can you make guppy-comptime enqueue the instantiations that it needs, and then do "another round" of checking (emptying the queue) after comptime?
| param_var_mapping: dict[str, Parameter] = field(default_factory=dict) | ||
|
|
||
| #: Type parameters that are bound to concrete arguments | ||
| param_inst: Mapping[str, Argument] = field(default_factory=dict) |
There was a problem hiding this comment.
This is what I was saying you should call generic_args earlier - but param_inst could be good too - and/or maybe a new type: it's just like Inst except keyed by string rather than int
There was a problem hiding this comment.
Ok I was not clear that the keys here were node id's!
…the CompilerContext
| """Returns the compiled definitions corresponding to the given ID, along with | ||
| the remaining type args that have not been monomorphized away. | ||
|
|
||
| Might mutate the current Hugr if this definition has never been compiled before. | ||
| """ |
There was a problem hiding this comment.
May be slightly outdated
# Conflicts: # guppylang-internals/src/guppylang_internals/checker/cfg_checker.py
On main, we only raise an error for decls that are generic over non-nat constants, most are accepted. With this PR, we now accept all of them. If we really wanted, we could preserve the behaviour of main, but imo it's not worth the complexity (we would need to keep the |
mark-koch
force-pushed
the
feat/early-mono
branch
from
40ccd49 to
1608826
Compare
|
| Branch | feat/early-mono |
| Testbed | Linux |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes x 1e3 (Result Δ%) | Upper Boundary nodes x 1e3 (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold | 141.59 x 1e3(-10.82%)Baseline: 158.77 x 1e3 | 160.36 x 1e3 (88.29%) | 📈 view plot 🚷 view threshold | 6.64 x 1e3(0.00%)Baseline: 6.64 x 1e3 | 6.71 x 1e3 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 25.96 x 1e3(-5.72%)Baseline: 27.53 x 1e3 | 27.81 x 1e3 (93.35%) | 📈 view plot 🚷 view threshold | 1.07 x 1e3(0.00%)Baseline: 1.07 x 1e3 | 1.08 x 1e3 (99.01%) |
This reverts commit bde8404.
This reverts commit bde8404.
)" This reverts commit bde8404.
5 participants
This PR updates to compiler to monomorphize every generic definition. The resulting Hugr will contain no more generic functions.
Monomorphization happens during type checking, so every version is type checked separately.
This is a precursor for #1299.
Also closes #1032 as a drive-by.
In particular, note the breaking internal change that
guppylang_internals.tys.subst.Instis now atupleinstead of aSequence.BREAKING CHANGE: Removed
MonomorphizableDefandMonomorphizedDef. UseGenericCheckableDefinstead.BREAKING CHANGE:
CompiledCallableDef.compile_callno longer takes type argsBREAKING CHANGE: Removed
CompiledCallableDef.load_with_args. UseCompiledCallableDef.loadinstead.BREAKING CHANGE: Removed
ParameterBase.to_hugrsince we no longer need to be able to turn params into Hugr paramsBREAKING CHANGE: Removed
PartiallyMonomorphizedArgs, partially_monomorphize_args,require_monomorphization,compile_variable_idxMonoArgs,CompilerContext.{current_mono_args, set_monomorphized_args, type_var_to_hugr, const_var_to_hugr},instantiation_needs_unpacking.BREAKING CHANGE:
Context.generic_paramsrenamed toContext.generic_inst, now storing an instantiation instead of parametersBREAKING CHANGE: Removed
NoneType.preserveandTupleType.preservefieldsBREAKING CHANGE:
Instis now atupleinstead of aSequenceBREAKING CHANGE:
CompilerContext.compilenow requires type argumentsBREAKING CHANGE:
CompilerContext.build_compiled_defno longer returns the remaining type argsBREAKING CHANGE:
CustomFunctionDefis no longer aCompiledCallableDef. Added a newCustomMonoFunctionDefthat is the result of checking aCustomFunctionDeffor some concrete type argsBREAKING CHANGE:
RawFunctionDecl.parsenow returns an intermediateParsedFunctionDeclthat is turned into a monomorphicCheckedFunctionDeclafter checkingBEGIN_COMMIT_OVERRIDE
feat: Monomorphise everything during type checking
END_COMMIT_OVERRIDE