nim-lang / compilerdev

This repository contains a collection of documents about how to change/refactor the Nim compiler in order to make it faster, easier to maintain and have fewer bugs by a superior architecture and design. However, no every idea here will work out.
10 stars 3 forks source link

Q: treat `result` as a normal variable? #14

Open stisa opened 4 years ago

stisa commented 4 years ago

I've been toying with writing a webassembly backend for nim for a while, and through that I'm learning more about nim, and I noticed that the result variable isn't actually declared in the ast, but seems just to be special cased in the codegen.

My question is: should we actually generate the ast for the declaration of result (and correspoding return result) in some previous pass (before the codegen)?

I managed to hack up transf.nim to add the result declaration in transformBody with this:

if prc.isIterator:
  result = g.transformClosureIterator(prc, result)
else:
  if prc.getReturnType.kind != tyNone:
    result = newTree(
      nkStmtList, 
      newTree(
        nkVarSection,
        newTree(
          nkIdentDefs,
          prc.ast[resultPos],
          newNodeIT(nkType, prc.info,prc.getReturnType),
          newNodeI(nkEmpty, prc.info)
        )
      ),
      result, 
      newTree(nkReturnStmt, prc.ast[resultPos])
    )

(I know this can be made shorter and the newTrees are probably unnecessary, and transf may be pretty late to do this transformation)
For a proc

proc a(x, y:int): int =
  x+y

this generates what I believe is the correct code (using renderTree on the result of transformBody):

proc a(x, y:int): int =
  var result: int
  result = x+y
  return result

which could then go through the normal codegen path for nkVarSection.

I don't think this would lead to performance gains, but it would make result less magic and more explicit, and probably slightly simplify code generation, which to me would still make this worthwhile.

Thoughts?

Araq commented 4 years ago

But result's declaration is accessible via n[resultPos].sym (check for resultPos < n.len before accessing it).

stisa commented 4 years ago

Yes, that's where I get the result symbol currently, I was just thinking it might be interesting to transform the body of the proc so that result is just a variable named result, as that way I wouldn't have to think about handling result at all.
It may lead to more complexity handling stuff like var T result though, so probably not worth the effort.

Also I saw some comments about wanting to require result = "" or similar, and doing the transformation could avoid having to write that in the actual code, but for that we could just generate the assignment in the transformation instead of the var declaration.

disruptek commented 3 years ago

Removing the special-casing makes some sense to me. What's the harm in it?

Araq commented 3 years ago

Well we need to patch the compiler and it's not clear how to access result. Consider that the codegen needs to turn proc p(): T = discard into proc p(): T = result = default(T)

disruptek commented 3 years ago

Looks like it prepends a var result = default(T) to the body (what else?) -- I think the cases where we add Result into proc params in codegen are more hairy than this, right?

Araq commented 3 years ago

I am not sure. However, there are use-cases where you want an easy way to access the result symbol in a typed macro. So once again, a spec for typed ASTs would be nice.

timotheecour commented 3 years ago

I like this RFC;

this is also related: https://github.com/nim-lang/Nim/issues/14420#issuecomment-633711557

> IMO, this issue is not VM specific. All backends introduce a temporary variable in this case.
IMO, solution is to introduce lowering in `transf` phase:
Transform:
```nim
x = block:
  stmt1
  stmt2
  expr

Into

block:
  stmt1
  stmt2
  x = expr

Similar lowering for if, block, case, stmtList expressions. injectdestuctors already does this lowering. It needs to be moved from injectdestructors to transf.


and this https://github.com/nim-lang/Nim/pull/16002#discussion_r530649897

> However, there are use-cases where you want an easy way to access the result symbol in a typed macro.

and a few other issues in which the special cased `result` causes issues (eg https://github.com/nim-lang/Nim/issues/13450)

@Araq 
> However, there are use-cases where you want an easy way to access the result symbol in a typed macro.

see this:
```nim
when true:
  import macros
  macro getTransf3(a: typed): string =
    newLit a.getImplTransformed.treeRepr
  proc baz(): int =
    if true: 1 else: 2
  echo getTransf3(baz)

prints:

ProcDef
  Sym "baz"
  Empty
  Empty
  FormalParams
    Sym "int"
  Empty
  Empty
  Asgn
    Sym "result"
    IntLit 1
  Sym "result"

under this RFC, we could still have Sym "result" inserted in resultPos position in AST, it doesn't change that IIUC

links

https://github.com/timotheecour/Nim/issues/385

disruptek commented 3 years ago

FWIW, CPS is currently using the result symbol from the procdef AST in order to correctly identify/rewrite the result symbol in the body supplied by the user. So there's an easy place to add a test or two...