Refactor/typed lifted

[b-studios]

No description provided.

[b-studios]

@marzipankaiser it would be very valuable to get a review from you. In particular, I simply duplicated DeclarationContext, which definitively needs a better solution.

[b-studios]

It looks like my last commit broke the build. However, I think it is only a symptom of something broken in machine. Function types should take one evidence parameter for the function itself -- this is how our translation currently work. Maybe machine assumed that it does not?

We get the following error (only introduced by changing the translation of types to add one evidence parameter for the function itself):

[info] running effekt.Server --backend llvm examples/llvm/recursive.effekt
opt: ./out/examples_llvm_recursive.ll:1323:9: error: multiple definition of local value named 'func_121'
        %func_121 = load %Neg, ptr %func_121p.129
        ^

@marzipankaiser Do you have an idea how this can happen?

[marzipankaiser]

@marzipankaiser it would be very valuable to get a review from you. In particular, I simply duplicated DeclarationContext, which definitively needs a better solution.

I don't really see how we can avoid that duplication as long as we have it in the Tree. The Scala types used by DeclarationContext in core/lifted are only related by name and structure IIUC. So either we'd need to change that somehow or use reflection/macros.

[marzipankaiser]

It looks like my last commit broke the build. However, I think it is only a symptom of something broken in machine. Function types should take one evidence parameter for the function itself -- this is how our translation currently work. Maybe machine assumed that it does not?

We get the following error (only introduced by changing the translation of types to add one evidence parameter for the function itself):

[info] running effekt.Server --backend llvm examples/llvm/recursive.effekt
opt: ./out/examples_llvm_recursive.ll:1323:9: error: multiple definition of local value named 'func_121'
        %func_121 = load %Neg, ptr %func_121p.129
        ^

@marzipankaiser Do you have an idea how this can happen?

Machine probably assumes the types it gets are core types, so the evidence parameters are added twice - once in lift inference and once by machine. At least that would be my first guess.

[b-studios]

Machine probably assumes the types it gets are core types, so the evidence parameters are added twice - once in lift inference and once by machine. At least that would be my first guess.

That's what I guessed, but I also touched all places where core.ValueType etc. was used and turned it into lifted.ValueType?

[b-studios]

@marzipankaiser do you think you could look into the machine/LLVM issue or do you think this is something @phischu needs to do? Is it difficult to check whether this also affects the JIT backend?

[marzipankaiser]

Machine probably assumes the types it gets are core types, so the evidence parameters are added twice - once in lift inference and once by machine. At least that would be my first guess.

That's what I guessed, but I also touched all places where core.ValueType etc. was used and turned it into lifted.ValueType?

Yes, but there are some places where we create evidence parameters from thin air in the machine translation IIRC.

[marzipankaiser]

@marzipankaiser do you think you could look into the machine/LLVM issue or do you think this is something @phischu needs to do? Is it difficult to check whether this also affects the JIT backend?

I am looking into it. If it affects LLVM, it will most likely affect JIT, too - I could try to merge them locally later.

[b-studios]

Yes, but there are some places where we create evidence parameters from thin air in the machine translation IIRC.

Do you have a particular place where you expect this to happen; I could not find it.

[marzipankaiser]

Turns out I was wrong: The problem seems to be the calculation of freeVariables (maybe related to #225) - e.g. in closures.effekt, the parameter g of f is returned by freeVariables on the block, and thus gets passed twice:

effekt/effekt/shared/src/main/scala/effekt/machine/Transformer.scala

Lines 70 to 82 in 1c1b4a0

	val freeParams = lifted.freeVariables(block).toList.collect {
	case lifted.ValueParam(id, tpe) => Variable(transform(id), transform(tpe))
	case lifted.BlockParam(id: (symbols.BlockParam | symbols.ResumeParam), tpe) =>
	// TODO find out if this is a block parameter or a function without inspecting the symbol
	Variable(transform(id), transform(tpe))
	case lifted.EvidenceParam(id) => Variable(transform(id), builtins.Evidence)
	// we ignore functions since we do not "close" over them.
	// TODO
	// case id: lifted.ScopeId => ???
	}
	val allParams = params.map(transform) ++ freeParams;
	noteBlockParams(id, allParams)

Here, (the same) g occurs both in params and in freeParams.

[marzipankaiser]

I think I found the issue:
Block parameters are not removed correctly from the free variables, as the types don't (never) match up between the use site and the parameter. They differ in the Id "self" (which prints the same but has a different .id)

[b-studios]

Thanks for the analysis @marzipankaiser. I turned the list of names for evidence parameters into a list of "Unit" (a natural number... / arity) to make sure structural equality still holds.

[b-studios]

As discussed in person, I am going ahead and merge this. We need to

1. rebase / merge into the mlton implementation
2. add explicit shift construct to lifted