code review 7202051: exp/ssa: (#3 of 5): Function, BasicBlock and optimisations

exp/ssa: (#3 of 5): Function, BasicBlock and optimisations

[adonovan, 2013-01-24 22:05:57]

Hello gri@google.com, iant@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2013-01-25 22:19:59]

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go
File src/pkg/exp/ssa/blockopt.go (right):

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:92: b.Preds = nil
Why bother clearing Preds and Succs?  Seems like they are about to become
unreachable anyhow.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:128: b.Succs = nil
Again, why bother clearing these fields?

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:143: restart:
It's not a big deal, but this could easily be written as
    changed := true
    for changed {
        changed = false
        ...
    }
The label and goto suggest (at least to me) that you have a more complex control
flow, but you don't.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/func.go
File src/pkg/exp/ssa/func.go (right):

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/func.go#newc...
src/pkg/exp/ssa/func.go:19: // Ask the terminal how wide it is.
Really doesn't seem like this belongs in this package.

[adonovan, 2013-01-25 22:41:51]

PTAL.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go
File src/pkg/exp/ssa/blockopt.go (right):

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:92: b.Preds = nil
On 2013/01/25 22:19:59, iant wrote:
> Why bother clearing Preds and Succs?  Seems like they are about to become
> unreachable anyhow.

Quite right; deleted.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:92: b.Preds = nil
On 2013/01/25 22:19:59, iant wrote:
> Why bother clearing Preds and Succs?  Seems like they are about to become
> unreachable anyhow.

Quite right; both deleted.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:128: b.Succs = nil
On 2013/01/25 22:19:59, iant wrote:
> Again, why bother clearing these fields?

Clearing b.Preds is how we make b unreachable (we can't nil out fn.Blocks[j]
because we don't know the index j for b).

I agree .Succs and .Instrs are overkill; I've removed them.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/blockopt.go#...
src/pkg/exp/ssa/blockopt.go:143: restart:
On 2013/01/25 22:19:59, iant wrote:
> It's not a big deal, but this could easily be written as
>     changed := true
>     for changed {
>         changed = false
>         ...
>     }
> The label and goto suggest (at least to me) that you have a more complex
control
> flow, but you don't.

Done.

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/func.go
File src/pkg/exp/ssa/func.go (right):

https://codereview.appspot.com/7202051/diff/4001/src/pkg/exp/ssa/func.go#newc...
src/pkg/exp/ssa/func.go:19: // Ask the terminal how wide it is.
On 2013/01/25 22:19:59, iant wrote:
> Really doesn't seem like this belongs in this package.

Agreed, but it really did make the output easier to read, and I can't think of a
"correct" approach (plumbing in this parameter from the outside world) that is
worth the effort.  

I've nuked it for now.  Sigh.

[iant, 2013-01-25 23:40:25]

LGTM

[adonovan, 2013-01-28 23:14:13]

*** Submitted as https://code.google.com/p/go/source/detail?r=250c03e820a7 ***

exp/ssa: (#3 of 5): Function, BasicBlock and optimisations

R=gri, iant, iant
CC=golang-dev
https://codereview.appspot.com/7202051