Skip to content

cmd/compile: complete all error-checking before SSA conversion #19250

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.

Sign up for GitHub

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

Jump to bottom
Closed
josharian opened this issue Feb 23, 2017 · 11 comments
Closed

cmd/compile: complete all error-checking before SSA conversion #19250

josharian opened this issue Feb 23, 2017 · 11 comments
Labels
FrozenDueToAge
Milestone
Go1.9Maybe

Comments

@josharian
Copy link
Contributor

There are a handful of checks that occur during gc -> SSA conversion, such as goto scoping. This is non-ideal for two reasons:

  • It will complicate making that conversion concurrent.
  • It means that we must do the expensive conversion step to fully check some code. We might want to stop early (CL 37384) for speed reasons (CL 37385) or e.g. to speed up fuzzing cmd/compile vs go/types.

I'd like to consider adding a compilation phase after walk but before SSA conversion to do all these error checks that would otherwise happen during conversion, assuming that that phase is cheap/fast enough.

cc @mdempsky @randall77 for opinions
@josharian josharian added this to the Go1.9Maybe milestone Feb 23, 2017
@dr2chase
Copy link
Contributor

I ran into this while tinkering with loop backedge rescheduling -- I could not willy-nilly rewrite loops and add labels, because my compiler-generated labels were sometimes flagged as "unused" -- but aren't we also intending to deprecate walk into non-existence?

@josharian
Copy link
Contributor Author

As I understand it, the plan for walk (and friends) is to be consumed from either side by the new parser and SSA. I'd like for us to ensure that any invalid code gets caught prior to doing the AST -> SSA conversion, whatever flavor the AST, and preserve that invariant. Step one is establishing that invariant. Sound ok?

@josharian
Copy link
Contributor Author

As for compiler-generated labels being flagged as unused, see the Node flag introduced (reused) in d1faf38.

@dr2chase
Copy link
Contributor

Your plan works for me.

@randall77
Copy link
Contributor

Sounds good to me.

@josharian josharian mentioned this issue Mar 3, 2017
Open
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38152 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38153 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 14, 2017
@josharian
cmd/compile: use Fatalf for more internal errors
50520f1 
There were a surprising number of places
in the tree that used yyerror for failed internal
consistency checks. Switch them to Fatalf.

Updates #15756
Updates #19250

Change-Id: Ie4278148185795a28ff3c27dacffc211cda5bbdd
Reviewed-on: https://go-review.googlesource.com/38153
Run-TryBot: Josh Bleecher Snyder <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
gopherbot pushed a commit that referenced this issue Mar 14, 2017
@josharian
cmd/compile: catch bad pragma combination earlier
5f5d882 
Bad pragmas should never make it to the backend.
I've confirmed manually that the error position is unchanged.

Updates #15756
Updates #19250

Change-Id: If14f7ce868334f809e337edc270a49680b26f48e
Reviewed-on: https://go-review.googlesource.com/38152
Run-TryBot: Josh Bleecher Snyder <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38159 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 15, 2017
@josharian
cmd/compile: check labels and gotos before building SSA
c03e75e 
This CL introduces yet another compiler pass,
which checks for correct control flow constructs
prior to converting from AST to SSA form.

It cannot be integrated with walk, since walk rewrites
switch and select statements on the fly.

To reduce code duplication, this CL also does some
minor refactoring.

With this pass in place, the AST to SSA converter
can now stop generating SSA for any known-dead code.
This minor savings pays for the minor cost of the new pass.

Performance is almost a wash:

name       old time/op     new time/op     delta
Template       206ms ± 4%      205ms ± 4%   ~     (p=0.108 n=43+43)
Unicode       84.0ms ± 4%     84.0ms ± 4%   ~     (p=0.979 n=43+43)
GoTypes        550ms ± 3%      553ms ± 3%   ~     (p=0.065 n=40+41)
Compiler       2.57s ± 4%      2.58s ± 2%   ~     (p=0.103 n=44+41)
SSA            3.94s ± 3%      3.93s ± 2%   ~     (p=0.833 n=44+42)
Flate          126ms ± 6%      125ms ± 4%   ~     (p=0.941 n=43+39)
GoParser       147ms ± 4%      148ms ± 3%   ~     (p=0.164 n=42+39)
Reflect        359ms ± 3%      357ms ± 5%   ~     (p=0.241 n=43+44)
Tar            106ms ± 5%      106ms ± 7%   ~     (p=0.853 n=40+43)
XML            202ms ± 3%      203ms ± 3%   ~     (p=0.488 n=42+41)

name       old user-ns/op  new user-ns/op  delta
Template        240M ± 4%       239M ± 4%   ~     (p=0.844 n=42+43)
Unicode         107M ± 5%       107M ± 4%   ~     (p=0.332 n=40+43)
GoTypes         735M ± 3%       731M ± 4%   ~     (p=0.141 n=43+44)
Compiler       3.51G ± 3%      3.52G ± 3%   ~     (p=0.208 n=42+43)
SSA            5.72G ± 4%      5.72G ± 3%   ~     (p=0.928 n=44+42)
Flate           151M ± 7%       150M ± 8%   ~     (p=0.662 n=44+43)
GoParser        181M ± 5%       181M ± 4%   ~     (p=0.379 n=41+44)
Reflect         447M ± 4%       445M ± 4%   ~     (p=0.344 n=43+43)
Tar             125M ± 7%       124M ± 6%   ~     (p=0.353 n=43+43)
XML             248M ± 4%       250M ± 6%   ~     (p=0.158 n=44+44)

name       old alloc/op    new alloc/op    delta
Template      40.3MB ± 0%     40.2MB ± 0%  -0.27%  (p=0.000 n=10+10)
Unicode       30.3MB ± 0%     30.2MB ± 0%  -0.10%  (p=0.015 n=10+10)
GoTypes        114MB ± 0%      114MB ± 0%  -0.06%  (p=0.000 n=7+9)
Compiler       480MB ± 0%      481MB ± 0%  +0.07%  (p=0.000 n=10+10)
SSA            864MB ± 0%      862MB ± 0%  -0.25%  (p=0.000 n=9+10)
Flate         25.9MB ± 0%     25.9MB ± 0%    ~     (p=0.123 n=10+10)
GoParser      32.1MB ± 0%     32.1MB ± 0%    ~     (p=0.631 n=10+10)
Reflect       79.9MB ± 0%     79.6MB ± 0%  -0.39%  (p=0.000 n=10+9)
Tar           27.1MB ± 0%     27.0MB ± 0%  -0.18%  (p=0.003 n=10+10)
XML           42.6MB ± 0%     42.6MB ± 0%    ~     (p=0.143 n=10+10)

name       old allocs/op   new allocs/op   delta
Template        401k ± 0%       401k ± 1%    ~     (p=0.353 n=10+10)
Unicode         322k ± 0%       322k ± 0%    ~     (p=0.739 n=10+10)
GoTypes        1.18M ± 0%      1.18M ± 0%  +0.25%  (p=0.001 n=7+8)
Compiler       4.51M ± 0%      4.53M ± 0%  +0.37%  (p=0.000 n=10+10)
SSA            7.91M ± 0%      7.93M ± 0%  +0.20%  (p=0.000 n=9+10)
Flate           244k ± 0%       245k ± 0%    ~     (p=0.123 n=10+10)
GoParser        323k ± 1%       324k ± 1%  +0.40%  (p=0.035 n=10+10)
Reflect        1.01M ± 0%      1.02M ± 0%  +0.37%  (p=0.000 n=10+9)
Tar             258k ± 1%       258k ± 1%    ~     (p=0.661 n=10+9)
XML             403k ± 0%       405k ± 0%  +0.47%  (p=0.004 n=10+10)

Updates #15756
Updates #19250

Change-Id: I647bfbb745c35630447eb79dfcaa994b490ce942
Reviewed-on: https://go-review.googlesource.com/38159
Run-TryBot: Josh Bleecher Snyder <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38973 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38971 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38972 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 30, 2017
@josharian
cmd/compile: cull unused ssa construction Error function
ebcb9cd 
The preceding passes have caught any errors
that could occur during SSA construction.

Updates #19250

Change-Id: I736edb2017da3f111fb9f74be12d437b5a24d2b4
Reviewed-on: https://go-review.googlesource.com/38971
Run-TryBot: Josh Bleecher Snyder <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
gopherbot pushed a commit that referenced this issue Mar 31, 2017
@josharian
cmd/compile: rework reporting of oversized stack frames
ca33e10 
We don't support stack frames over 2GB.
Rather than detect this during backend compilation,
check for it at the end of compilation.
This is arguably a more accurate check anyway,
since it takes into account the full frame,
including local stack, arguments, and arch-specific
rounding, although it's unlikely anyone would ever notice.

Also, rather than reporting the error right away,
take note of it and report it later, at the top level.
This is not relevant now, but it will help with making
the backend concurrent, as the append to the list of
oversized functions can be cheaply protected by a plain mutex.

Updates #15756
Updates #19250

Change-Id: Id3fa21906616d62e9dc66e27a17fd5f83304e96e
Reviewed-on: https://go-review.googlesource.com/38972
Run-TryBot: Josh Bleecher Snyder <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Cherry Zhang <[email protected]>
@golang golang locked and limited conversation to collaborators Mar 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
4 participants
@josharian @dr2chase @randall77 @gopherbot