Skip to content
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.

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

Q: treat result as a normal variable? #14

Open
stisa opened this issue Aug 30, 2020 · 8 comments
Open

Q: treat result as a normal variable? #14

stisa opened this issue Aug 30, 2020 · 8 comments

Comments

@stisa
Copy link

stisa commented Aug 30, 2020

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?

@stisa stisa changed the title Q: treat result as an injected variable? Q: treat result as a normal variable? Aug 30, 2020
@Araq
Copy link
Member

Araq commented Aug 31, 2020

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

@stisa
Copy link
Author

stisa commented Sep 1, 2020

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
Copy link

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

@Araq
Copy link
Member

Araq commented Dec 17, 2020

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
Copy link

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
Copy link
Member

Araq commented Dec 18, 2020

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
Copy link
Member

timotheecour commented Jan 13, 2021

I like this RFC;

this is also related: nim-lang/Nim#14420 (comment)

> 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
```nim
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 nim-lang/Nim#16002 (comment)

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 nim-lang/Nim#13450)

@Araq

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

see this:

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

timotheecour/Nim#385

@disruptek
Copy link

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants