Change the order of compilation passes, transformation is made lazy at code gen #8489

cooldome · 2018-07-31T22:29:29Z

Ready for the first review.

Original goal was to make getImpl return semantically checked AST but before transf pass.
Transformations can change regularly (examples: closure iterators and destructors) and user does not need to know about them. I have added getImplTransformed for backwards compatibility, in case someone needs it and requires time to migrate.

Performance numbers are actually pleasing:

Running koch boot -d:release second time (no invocation of c compiler).

Before:

  iteration 2: (116369 lines compiled; 8.146 sec total; 423.016MiB peakmem; Release Build)
  iteration 3: (116369 lines compiled; 7.532 sec total; 422.594MiB peakmem; Release Build)

After:

  iteration 2 :(116506 lines compiled; 7.563 sec total; 340.949MiB peakmem; Release Build)
  iteration 3: (116506 lines compiled; 7.104 sec total; 340.949MiB peakmem; Release Build)

I have noticed much bigger performance improvements for projects importing a lot but using a little. Quite common these days. Nim compiler is not one of them.

I also noticed startup time for nimsuggest improved.

P.S.
I have made also a good progress to reorder transf and lambdalifting passes, almost everything works
except closure iterators work. Changes in this PRs are required to make it happen anyway, it is easier
to split the work and unblock my project.

Araq · 2018-07-31T23:13:18Z

Sure, but it already fails at bootstrapping and that's usually easy enough to test locally. ;-)

cooldome · 2018-09-02T20:12:04Z

This now bootstrap's compiler.
I will look into remaining failures

cooldome · 2018-09-11T07:55:35Z

checkConstructedType it is. Added a change

cooldome · 2018-09-26T07:03:18Z

Could you please review this change? Is it the release that blocks the review, in this it can wait?

Araq · 2018-09-26T07:43:10Z

Could you please review this change?

I reviewed it, it's mostly fine but
a) will be merged after the release
b) has merge conflicts.

cooldome · 2018-10-06T16:35:30Z

I have merged the conflicts, ready for review.

There is one problem, travis has failed with error that seems related to my change. But I can't replicate it on any of my machines. Need help to progress, can't travis give at least stack trace of the problem's location? Can somesome try on their linux desks and say where is the problem if any?

Much appreciated.

Araq · 2018-10-09T13:08:52Z

The failure is:

�[1m�[31mFAIL: �[36mnimprof.nim C�[0m
�[1m�[36mTest "lib/pure/nimprof.nim" in category "lib"�[0m
�[1m�[31mFailure: reNimcCrash�[0m
�[33mExpected:�[0m
�[1m
�[0m
�[33mGotten:�[0m
�[1mtype mismatch: got <proc (): bool{.gcsafe, locks: <unknown>.}> but expected 'proc (): bool{.gcsafe, locks: 0.}'
�[0m

Araq · 2018-10-14T09:08:58Z

Now fails with cannot open 'C:\projects\nim\tests\testament\tester.nim', please rebase I want to merge this asap now, my move optimizer also would be better with this refactoring. :-)

cooldome · 2018-10-18T10:33:33Z

Ready for review.
I had to split tprevent_assign2 test into 2 tests, because transformation is now done at codegen and nim check does do codegen hence does not call destructor inject code.

I had to add silly locks: 0 to nimprof as I simply can't replicate this bug on any of my machines. It just works as expected for me so I can't progress on the better fix. If someone will post a better test case I will jump onto it.

Araq · 2018-10-18T12:55:10Z

compiler/semtypinst.nim

@@ -30,6 +30,11 @@ proc checkConstructedType*(conf: ConfigRef; info: TLineInfo, typ: PType) =
    localError(conf, info, "type 'var var' is not allowed")
  elif computeSize(conf, t) == szIllegalRecursion:
    localError(conf, info,  "illegal recursion in type '" & typeToString(t) & "'")
+
+  t = typ.skipTypes({tyGenericInst})
+  if t.kind == tyArray and tfUncheckedArray in t.flags:


tyUncheckedArray here?

Araq · 2018-10-18T12:57:23Z

lib/pure/nimprof.nim

@@ -145,7 +145,7 @@ else:
    else:
      dec gTicker

-  proc hook(st: StackTrace) {.nimcall.} =
+  proc hook(st: StackTrace) {.nimcall locks: 0.} =


Minor: use a comma here.

Araq · 2018-10-18T12:58:45Z

I had to split tprevent_assign2 test into 2 tests, because transformation is now done at codegen and nim check does do codegen hence does not call destructor inject code.

That's not good but we'll find a better solution for this later. :-)

…t code gen (nim-lang#8489) * Ast no transformation * Add getImplNoTransform to the macros module * progress on delaying transf * Fix methods tranformation * Fix lazy lambdalifting * fix create thread wrapper * transform for lambda lifting * improve getImplTransformed * Fix destructor tests * try to fix nimprof for linux

krux02 · 2020-05-19T23:58:52Z

lib/core/macros.nim

@@ -237,6 +237,10 @@ else: # bootstrapping substitute
    else:
      n.strValOld

+when defined(nimSymImplTransform):
+  proc getImplTransformed*(symbol: NimNode): NimNode {.magic: "GetImplTransf", noSideEffect.}
+    ## for a typed proc returns the AST after transformation pass


To this day, there is not a single test for this proc in the entire test suite.

fixing it in 14924

cooldome added 12 commits July 20, 2018 16:45

Ast no transformation

f0284e6

progress

62deebf

bug fix

4dd3826

cleanup

245ea8b

Add getImplNoTransform to the macros module

5861aae

Progress

2ed6cc4

merge devel

cc010a0

fix spacing

7aaae42

revert uintended change

904b199

progress on delaying transf

4ad02aa

Fix methods tranformation

2271178

Fix lazy lambdalifting

4b0e51e

cooldome changed the title ~~[WIP] Change the order compilation passes, tranformation is made lazy at code gen~~ [WIP] Change the order of compilation passes, transformation is made lazy at code gen Jul 31, 2018

cooldome added 12 commits August 1, 2018 23:18

progress

9d8da19

fix create thread wrapper

3520c03

transform for lambda lifting

9d2f497

progress

d7e4f43

debug

e7d8e68

Progress

e5b1d3a

progress

19b3b55

Progress

84af51a

merge devel

10fc2fa

boostrapping works

b8fc10c

cleanup

936fe8e

revert tmacrostmt

9fb8e8a

cooldome added 3 commits September 2, 2018 23:11

progress

84be822

Cleanup progress

c65438f

cleanup

73ffa09

cooldome added 2 commits September 10, 2018 23:30

bug fix

a7de9c4

move unchecked range

f76b4ca

merge devel

d256c22

This was referenced Oct 6, 2018

Render for loops in typed procs #8398

Closed

getImpl in macros before transformation pass. Helps with for loops rendering #8455

Closed

cooldome added 2 commits October 11, 2018 09:03

merge devel

f1aa1a9

Merge branch 'devel' into change_pass_order

4ff3320

Araq added the Next Release label Oct 14, 2018

cooldome added 7 commits October 15, 2018 09:29

Merge branch 'devel' into change_pass_order

c914262

fix test

a325bab

fix test

380ac31

Merge remote-tracking branch 'upstream/devel' into devel

ef42694

Fix destructor tests

6ad72a2

try to fix nimprof for linux

ef7ac9d

merge devel

efa1940

Araq reviewed Oct 18, 2018

View reviewed changes

Araq merged commit eaca5be into nim-lang:devel Oct 18, 2018

This was referenced Oct 18, 2018

Minor bug fix #9430

Merged

Transf minor code optimization #9433

Merged

krux02 reviewed May 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the order of compilation passes, transformation is made lazy at code gen #8489

Change the order of compilation passes, transformation is made lazy at code gen #8489

cooldome commented Jul 31, 2018 •

edited

Araq commented Jul 31, 2018

cooldome commented Sep 2, 2018

cooldome commented Sep 11, 2018 •

edited

cooldome commented Sep 26, 2018

Araq commented Sep 26, 2018

cooldome commented Oct 6, 2018

Araq commented Oct 9, 2018

Araq commented Oct 14, 2018

cooldome commented Oct 18, 2018

Araq Oct 18, 2018

Araq Oct 18, 2018

Araq commented Oct 18, 2018

krux02 May 19, 2020

timotheecour Jul 7, 2020 •

edited

Change the order of compilation passes, transformation is made lazy at code gen #8489

Change the order of compilation passes, transformation is made lazy at code gen #8489

Conversation

cooldome commented Jul 31, 2018 • edited

Araq commented Jul 31, 2018

cooldome commented Sep 2, 2018

cooldome commented Sep 11, 2018 • edited

cooldome commented Sep 26, 2018

Araq commented Sep 26, 2018

cooldome commented Oct 6, 2018

Araq commented Oct 9, 2018

Araq commented Oct 14, 2018

cooldome commented Oct 18, 2018

Araq Oct 18, 2018

Choose a reason for hiding this comment

Araq Oct 18, 2018

Choose a reason for hiding this comment

Araq commented Oct 18, 2018

krux02 May 19, 2020

Choose a reason for hiding this comment

timotheecour Jul 7, 2020 • edited

Choose a reason for hiding this comment

cooldome commented Jul 31, 2018 •

edited

cooldome commented Sep 11, 2018 •

edited

timotheecour Jul 7, 2020 •

edited