Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactorings to prepare the compiler for IC #15935

Merged
merged 32 commits into from
Dec 17, 2020
Merged

refactorings to prepare the compiler for IC #15935

merged 32 commits into from
Dec 17, 2020

Conversation

Araq
Copy link
Member

@Araq Araq commented Nov 12, 2020

Also speeds up the compiler in general, saved 0.4s from bootstrapping.

Todo:

  • enable converters and TR macros.
  • make every test green.

@planetis-m
Copy link
Contributor

planetis-m commented Dec 15, 2020

so my code that breaks looks like:

proc rnd[T: SomeFloat](x: int, v: T) = discard
template rnd[T](x: int) = rnd[T](x, T(1))
proc rnd2[T: SomeFloat](x: int, v: T) = discard
template rnd2[T](x: int) = rnd2[T](x, T(1))

I remember having hard-to-pin-down bugs with these templates before, (with my custom destructor hooks not getting used) and the workaround was to move the templates below each function!? Which was weird but worked.

@planetis-m
Copy link
Contributor

Yeah I'm getting expression 'randMatrix[float32](rowDimension(A), columnDimension(A))' cannot be called with devel nim by moving the template.

@Araq
Copy link
Member Author

Araq commented Dec 15, 2020

Yeah I'm getting expression 'randMatrix[float32](rowDimension(A), columnDimension(A))' cannot be called with devel nim by moving the template.

Yes, I know. It's easy to reproduce and blocking this merge:

type
  Matrix[T] = object
    data: T


proc randMatrix*[T](m, n: int, max: T): Matrix[T] = discard
proc randMatrix*[T](m, n: int, x: Slice[T]): Matrix[T] = discard

template randMatrix*[T](m, n: int): Matrix[T] = randMatrix[T](m, n, T(1.0))

let B = randMatrix[float32](20, 10)

tree.nodes.add Node(kind: nkSym, operand: int32(s), info: info)

proc addModuleId*(tree: var PackedTree; s: ModuleId; info: PackedLineInfo) =
tree.nodes.add Node(kind: nkInt32Lit, operand: int32(s), info: info)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use directIntLit her; even if the same right now, it's more readable

nkUInt8Lit,
nkUInt16Lit,
nkUInt32Lit,
nkUInt64Lit} # nkInt32Lit is missing by design!
Copy link
Member

@timotheecour timotheecour Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the benefit of adding this odd special case?
why not add a new nkDirectIntLit instead and avoid conflating an int32 with this; the saving is insignificant but it will lead to less bugs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree but I cannot easily extend the AST without touching the macro system.

Copy link
Member

@timotheecour timotheecour Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you can add to the end, it shouldn't break code (and other node kinds have been added in past without issues)

for now it maybe doesn't even need to be mapped to macros.NimNodeKind, but if needed it can with when defined(nimHasNkDirectIntLit) (can be DRY with nim-lang/RFCs#190, or simply with duplication)

externUIntLit* = {nkUIntLit, nkUInt8Lit, nkUInt16Lit, nkUInt32Lit, nkUInt64Lit}
directIntLit* = nkInt32Lit

proc toString*(tree: PackedTree; n: NodePos; nesting: int; result: var string) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(+ elsewhere)
result: var string should go first, more consistent with rest of language/stdlib and works better with dup + friends

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I don't know if I'll get around to it in this PR.

ModulePhase* = enum
preLookup, lookedUpTopLevelStmts

Module* = object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this be a ref object? it should have ref semantics

@timotheecour
Copy link
Member

timotheecour commented Dec 16, 2020

If the main goal is IC and serialization of modules, why can't we simply just add functionality to serialize/deserialize the existing modules (PNode, PSym, PType etc) instead of introducing a totally parallel IR that duplicates existing data structures and must co-exist with it for the foreseeable future without actually replacing it.

This would result in a much less invasive change compared to this PR.

As for the stated benefits of packed AST (vs existing unpacked one) in terms of performance, they'd need to be measured carefully; I'm not convinced it'd justify the added complexity; the AST is after all representing a tree so representing it as a packed structure leads to impedance mismatch. In any case that's not apparently the main goal, which is IC (IC itself, not packed AST, will deliver large performance improvements for compile times).

The changes here are quite invasive (and will cause lots of PRs to break and go back to drawing board) so there needs to be a tangible benefit vs alternatives.

Also speeds up the compiler in general, saved 0.4s from bootstrapping.

I'm not observing this, I get the same timings modulo noise.
before PR, ./koch boot -d:release takes 47.8 (similar if re-running a few times)
after PR, ./koch boot -d:release takes 47.5 (similar if re-running a few times)

But any such savings would be dwarfed by the gains from doing incremental compilation.

different approach for IC: compiler server

I've actually implemented a different approach based on compilation as service; it's a lot less invasive and has following features, inspired by nimsuggest:

  • a long living server process that listens through a port; it holds data structures (module data etc) in memory
  • a short lived client that users call from command line to compile / incrementally re-compile a nim project; it communicates through RPC to the server

I've used it for a working proof of concept of a REPL that can be used similar to inim except it's really fast (similar speed to nim secret, but unlike nim secret it generates/runs cgen'd code, not vm code, so importc works etc) as it doesnt' recompile everything on each new issued command, instead it (for now at least) creates 1 new module for each new command and all existing data / imported modules don't need to be semchecked again. There are still unknowns but it looks really promising, in between nim secret and inim. And it doesn't require any serialization/deserialization either.

@Araq
Copy link
Member Author

Araq commented Dec 16, 2020

before PR, ./koch boot -d:release takes 47.8 (similar if re-running a few times)
after PR, ./koch boot -d:release takes 47.5 (similar if re-running a few times)

So you observe a 0.3s difference. :-)

As for the stated benefits of packed AST (vs existing unpacked one) in terms of performance, they'd need to be measured carefully; I'm not convinced it'd justify the added complexity; the AST is after all representing a tree so representing it as a packed structure leads to impedance mismatch. In any case that's not apparently the main goal, which is IC (IC itself, not packed AST, will deliver large performance improvements for compile times).

The way we modelled the AST is a directed acyclic graph, not as a tree. With bad mutability tracking and hacks. In the packed representation it's a tree by construction, you cannot create cycles or share nodes, even if you wanted to. It does take up less memory and it really is faster to process, but that's not even the main goal, it's about "correct by construction". And the risks are close to zero, for complex transformations you can convert it to PNode, do the transform and pack it again afterwards.

Also, it is my hope that with the packed representation we can compute hashes more effectively, for example, to quickly deduplicate function bodies in the backend or to cache generic instantiations.

The changes here are quite invasive (and will cause lots of PRs to break and go back to drawing board) so there needs to be a tangible benefit vs alternatives.

I don't see it this way. There is a refactoring so that module imports are now done lazily. Something that we really need when we seek to load modules from disk. The PackedTree is not used anywhere yet, it's part of this PR so that I don't lose this code. If we end up not using it, so be it.

different approach for IC: compiler server

You are not the first who proposes this. In my experience it works so bad for nimsuggest that I don't want to go down this path. It's also inferior, I like to save compilation artifacts on disk, not just in memory. The way we currently do it is a dead-end, you have this fragile process and when it crashes most of the information that caused the crash is lost. It's also not how the rest of the industry does it (precompiled headers, FPC's ppu files, C#'s bytecode).

@Araq
Copy link
Member Author

Araq commented Dec 16, 2020

@planetis-m Please update your package so that it uses:

proc randMatrix*[T](m, n: int): Matrix[T] {.inline.} = randMatrix[T](m, n, T(1.0))
proc randMatrix32*(m, n: int): Matrix32 {.inline.} = randMatrix(m, n, 1.0'f32)
proc randMatrix64*(m, n: int): Matrix64 {.inline.} = randMatrix(m, n, 1.0)

It only works by chance with Nim devel and we're tracking the bug here, #16376

@planetis-m
Copy link
Contributor

@Araq done.

@Araq Araq merged commit 979148e into devel Dec 17, 2020
@Araq Araq deleted the araq-ic4 branch December 17, 2020 07:01
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* added ic specific Nim code; WIP
* make the symbol import mechanism lazy; WIP
* ensure that modules can be imported multiple times
* ambiguity checking
* handle converters and TR macros properly
* make 'enum' test category green again
* special logic for semi-pure enums
* makes nimsuggest tests green again
* fixes nimdata
* makes nimpy green again
* makes more important packages work
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* added ic specific Nim code; WIP
* make the symbol import mechanism lazy; WIP
* ensure that modules can be imported multiple times
* ambiguity checking
* handle converters and TR macros properly
* make 'enum' test category green again
* special logic for semi-pure enums
* makes nimsuggest tests green again
* fixes nimdata
* makes nimpy green again
* makes more important packages work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants