compiler: more progress on incremental #19273
Conversation
mlugg
force-pushed
the
incremental-some-more
branch
4 times, most recently
from
deabaa8 to
9f0d142
Compare
There is no reason to perform this detection during semantic analysis. In fact, doing so is problematic, because we wish to utilize detection of existing decls in a namespace in incremental compilation.
This makes tracking easier across incremental updates: `scanDecl` can now tell whether an existing decl in a namespace was mapped to the one it's analyzing in the new ZIR.
Most basic re-analysis logic is now in place. Trivial updates are hitting linker assertions.
This was an oversight in my original design. This new form of dependency is invalidated when the resolved IES for a runtime function changes.
See comment. This slightly regresses a previous fix from this branch. A proper solution will come soon, with the splitting up of `Decl` into `Cau` and `Nav`.
mlugg
force-pushed
the
incremental-some-more
branch
from
9f0d142 to
7c32370
Compare
| // Do a pass over the namespace contents and remove any decls from the last update | ||
| // which were removed in this one. | ||
| var i: usize = 0; | ||
| while (i < namespace.decls.count()) { |
There was a problem hiding this comment.
4294 should make it unnecessary but 4295 would immediately crash
There was a problem hiding this comment.
Oops, you're totally right, good spot! Luckily this doesn't affect code built without incremental, and this is going to be changed a lot again soon anyway, but thank you for pointing out the issue.
|
In case you didn't think of it already, a nice stepping stone might be an incremental Furthermore - and this is the cool part - once frontend incremental is robust we could enable it by default when |
|
Yeah, that thought has crossed my mind. I need to step back and do some prereq work for now - i.e. mutilating comptime-mutable memory and |
| const decl = zcu.declPtr(decl_index); | ||
| if (!seen_decls.contains(decl.name)) { | ||
| // We must preserve namespace ordering for @typeInfo. | ||
| namespace.decls.orderedRemoveAt(i); |
There was a problem hiding this comment.
You can't put a orderedRemove() inside a loop. That's O(N^2). I would have considered this to be a merge blocker.
Looks like this is some big strides forward otherwise though. Nice! 🙂
There was a problem hiding this comment.
Oops, apologies - I didn't think of that. This code is only hit in the incremental path, and it'll probably be changed again anyway after the Decl changes, but I'll make sure this gets fixed!
We're deep in the weeds now. As of this PR trivial updates are blocked on linker progress (or CBE/LLVM backend progress), but I can still make progress in the meantime.
Before I make more steps towards incremental, I need to have a stern talking-to with
Zcu.Decl(see my proposed refactor) - but I think we're very close to something usable on the frontend.