Skip to content

186 compile blocks using druid #192

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

luc-raz
Copy link
Contributor

@luc-raz luc-raz commented May 16, 2025

First attempt to compile block on DRUID:

  • like in a method, we create a tempVector to store the args and temporaries of the block
  • also, we send the outer temp Vector, if one of its temps is modified inside the block (don't work yet when reading temp)

To do:

  • Compile nested block
  • always send the outer Temp Vector to a block even if just reading variable

Fix #186

Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I made some comments.

Generally speaking:

  • some method names could be improved
  • lots of commented code and halts around

@@ -69,7 +69,6 @@ DRBlockClosure >> executeOn: interpreter [
{ #category : 'interpreting' }
DRBlockClosure >> generateCFG [

self error: #ToImplement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D

Comment on lines 2122 to 2130
DruidTestInterpreter >> simpleMethodWithBlockReadingOuterVariable [

| temp |
temp:=0.
self basicMethodWithBlock: [ |x|.
temp:=x.
x].

^ temp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is not reading the outer variable, its writing on it...

Comment on lines +2109 to +2115
DruidTestInterpreter >> simpleMethodWithBlock [

| temp |
temp:=0.
self basicMethodWithBlock: [ | x |
x:=5.
x].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is reading it

Comment on lines 371 to 373
vectorTempName := aDRLoadTemporaryVariable isTempVectorTemp
ifTrue: [ self vectorTempName ]
ifFalse: [ 'vector1' ].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicated with the code in visitStoreTempVar:


^ self instructions select: [ :i | i isStoreTemporaryVariable ]
^ self instructions select: [ :i | (i isStoreTemporaryVariable or: [i isLoadTemporaryVariable]) "and: [i isTempVectorTemp not]"].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented code

Comment on lines 31 to 32
"optimisations add: DRInline new.
optimisations add: DRInline new."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented code!


aRBAssignmentNode variable binding originalVar isTempVectorTemp ifTrue: [
| result |
"maybe should create DRStoreVectorTemporaryVariable ???"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes?
Otherwise, how do you differentiate normal stores?

Or for now all stores go to the temp vector?

Comment on lines -188 to +202
withState: executionState.
aRBVariableNode variable isEscaping ifFalse: [ "should use isCopying"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should use bien isCopying

@luc-raz
Copy link
Contributor Author

luc-raz commented May 19, 2025

For now, all arguments and temporaries are all stored in temp Vector.
And temp vector name are hardcoded: for the block it comes from the scope.
We have two options:

  • Create our own scope for DRUID
  • Use the existing own and try to modify it (it doesn't have accessors for tempVector, copiedVars, tempVars

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

Successfully merging this pull request may close these issues.

Compile Blocks using Druid
2 participants