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

Graph API work contd. #297

Merged
merged 13 commits into from Jan 7, 2020
Merged

Graph API work contd. #297

merged 13 commits into from Jan 7, 2020

Conversation

@cwgoes
Copy link
Collaborator

cwgoes commented Jan 6, 2020

test_eval_jit in test/Backends/LLVM.hs should suffice for print testing.

cwgoes added 2 commits Jan 6, 2020
@cwgoes

This comment has been minimized.

Copy link
Collaborator Author

cwgoes commented Jan 6, 2020

Presently, when JIT-ing the initial module:

juvix-test: EncodeException "A type definition requires a structure type but got: IntegerType {typeBits = 64}"
@cwgoes

This comment has been minimized.

Copy link
Collaborator Author

cwgoes commented Jan 6, 2020

The problem is numPorts, which is defined as an integer if the address space is large enough.

cwgoes added 5 commits Jan 6, 2020
@cwgoes cwgoes requested a review from mariari Jan 6, 2020
@cwgoes cwgoes marked this pull request as ready for review Jan 6, 2020
cwgoes added 3 commits Jan 6, 2020
Copy link
Contributor

mariari left a comment

Overall seems fine, could use just a bit of touchups

src/Juvix/Backends/LLVM/Net/API.hs Outdated Show resolved Hide resolved
src/Juvix/Backends/LLVM/Net/Environment.hs Outdated Show resolved Hide resolved
src/Juvix/Backends/LLVM/Net/API.hs Outdated Show resolved Hide resolved
src/Juvix/Backends/LLVM/Net/API.hs Outdated Show resolved Hide resolved
src/Juvix/Backends/LLVM/Net/API.hs Outdated Show resolved Hide resolved
src/Juvix/Backends/LLVM/Codegen/Types.hs Outdated Show resolved Hide resolved
@@ -23,3 +23,7 @@ tmp/

# Agda temporary
*.agdai

# LLVM temporary
*.ll

This comment has been minimized.

Copy link
@mariari

mariari Jan 6, 2020

Contributor

Could probably be removed once we have a stable directory for outputs to go in

This comment has been minimized.

Copy link
@cwgoes

cwgoes Jan 6, 2020

Author Collaborator

Yeah, but for now it's convenient.

Primar Erase 0
Auxiliary2 Lambda 1
Auxiliary2 App 2
Auxiliary2 (FanIn i) i
--INIR.nodePorts = map (\(_, toNode, toPort) → INIR.Port toNode (portTypeToIndex toPort)) edges
Comment on lines +104 to +108

This comment has been minimized.

Copy link
@mariari

mariari Jan 6, 2020

Contributor

This looks non exhaustive?

This comment has been minimized.

Copy link
@cwgoes

cwgoes Jan 6, 2020

Author Collaborator

It is, to-do for a future PR.

This comment has been minimized.

Copy link
@cwgoes

cwgoes Jan 6, 2020

Author Collaborator

(still need to determine the exact memory model to pass through to LLVM)

-- Exit case: next loop.
Codegen.setBlock forExit
Codegen.retNull
Codegen.store counter (Operand.ConstantOperand (C.Int 32 0))

This comment has been minimized.

Copy link
@mariari

mariari Jan 6, 2020

Contributor

Why store 0 on the counter?

This comment has been minimized.

Copy link
@cwgoes

cwgoes Jan 6, 2020

Author Collaborator

It's being re-used for a second loop.

Codegen.br forLoop2
-- Second loop: link nodes.
Codegen.setBlock forLoop2
ind Codegen.load int32 counter

This comment has been minimized.

Copy link
@mariari

mariari Jan 6, 2020

Contributor

Since we store 0 on the counter before coming here, this should always be 0. Thus indexs here are on 0?

This comment has been minimized.

Copy link
@cwgoes

cwgoes Jan 6, 2020

Author Collaborator

It's incremented later (line 155) - the blocks are not in execution order (confusingly I suppose)

This comment has been minimized.

Copy link
@mariari

mariari Jan 7, 2020

Contributor

Well the reason why I found this odd, is because you jump to the wrong loop, see below

This comment has been minimized.

Copy link
@mariari

mariari Jan 7, 2020

Contributor

Well, the reason it seems odd, is that when we hit this point, we only ever add 1 to it before never touching this part again. See comments below

cwgoes and others added 3 commits Jan 6, 2020
Co-Authored-By: mariari <32526923+mariari@users.noreply.github.com>
@cwgoes cwgoes dismissed mariari’s stale review Jan 7, 2020

Requests addressed.

@cwgoes cwgoes merged commit 969cbd9 into develop Jan 7, 2020
2 checks passed
2 checks passed
ci/circleci: setup_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_suite Your tests passed on CircleCI!
Details
@cwgoes cwgoes deleted the cwgoes/graph-api-work branch Jan 7, 2020
Copy link
Contributor

mariari left a comment

Seems some minor jump errors

Codegen.br forLoop2
-- Second loop: link nodes.
Codegen.setBlock forLoop2
ind Codegen.load int32 counter

This comment has been minimized.

Copy link
@mariari

mariari Jan 7, 2020

Contributor

Well the reason why I found this odd, is because you jump to the wrong loop, see below

src/Juvix/Backends/LLVM/Net/API.hs Show resolved Hide resolved
Codegen.br forLoop2
-- Second loop: link nodes.
Codegen.setBlock forLoop2
ind Codegen.load int32 counter

This comment has been minimized.

Copy link
@mariari

mariari Jan 7, 2020

Contributor

Well, the reason it seems odd, is that when we hit this point, we only ever add 1 to it before never touching this part again. See comments below

next Codegen.add int32 ind (Operand.ConstantOperand (C.Int 32 1))
Codegen.store counter next
cond Codegen.icmp IntPred.EQ node_count next
Codegen.cbr cond forLoop forExit

This comment has been minimized.

Copy link
@mariari

mariari Jan 7, 2020

Contributor

Codegen.cbr cond forLoop2 forExit2

this is probably what you wanted, with this, your increment would be correct

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.