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

[adon...@google.com]

Reviewers: gri1, iant2,

Message:
Hello g...@google.com, ia...@google.com (cc: golan...@googlegroups.com),

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


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

Please review this at https://codereview.appspot.com/7202051/

Affected files:
A src/pkg/exp/ssa/blockopt.go
A src/pkg/exp/ssa/func.go

[ia...@golang.org]

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#newcode92
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#newcode128
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#newcode143
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#newcode19
src/pkg/exp/ssa/func.go:19: // Ask the terminal how wide it is.
Really doesn't seem like this belongs in this package.

https://codereview.appspot.com/7202051/

[adon...@google.com]

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#newcode92
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#newcode92
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#newcode128
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#newcode143
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#newcode19
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.

https://codereview.appspot.com/7202051/

[ia...@golang.org]

LGTM

https://codereview.appspot.com/7202051/