Conversation
PalumboN
left a comment
PalumboN
left a comment
There was a problem hiding this comment.
Hi @luc-raz ,
I left some comments, answer them if you want to discuss something.
The main issue are the tests (that are not calling the primitive) and some code cleaning.
The thing to think about is the model of the pragmas in our IR. I think that an instruction per pragma is better, but I'm not sure.
| { #category : 'visiting' } | ||
| DRBlockIRGenerator >> interpretCode: aDRMethod receiver: aReceiver [ | ||
|
|
||
| self setupCFGScope: aDRMethod codeNode. | ||
|
|
||
| self pushFrameForCode: aDRMethod receiver: aReceiver. | ||
| aDRMethod executeOn: self. | ||
|
|
||
| ^ self popFrame. | ||
|
|
||
| ] | ||
|
|
There was a problem hiding this comment.
This method is already implemented in the superclass DRMethodIRGenerator
There was a problem hiding this comment.
On the super method, the difference is I initialize the pragma
There was a problem hiding this comment.
But you are not doing that in this method 🤔
| { #category : 'queries' } | ||
| DRMethodControlFlowGraph >> initialize [ | ||
|
|
||
| super initialize . | ||
| ] | ||
|
|
| { #category : 'initialization' } | ||
| DRInstructionFactory >> initializePragma [ | ||
|
|
||
| ^ DRInitializePragma pragmas. | ||
| ] | ||
|
|
||
| { #category : 'initialization' } | ||
| DRInstructionFactory >> initializeScope: aDRScope [ | ||
|
|
||
| ^ DRInitializeScope scope: aDRScope | ||
| ] | ||
|
|
There was a problem hiding this comment.
These methods must be extension methods of Druid-Opal package, because these classes are in that package. And sometimes we want to load Druid code without the Opal extension ;)
| "if the pragma is a primitive that defines an error variable, we need to store error value" | ||
| each isPrimitiveErrorPragma ifTrue: [ | ||
| builder | ||
| storeRemoteTemp: each primitiveErrorVariableName | ||
| inVector: aDRInitializePragma scope tempVectorName.] |
There was a problem hiding this comment.
Ahh the error variable is working then? 🤔
| { #category : 'accessing' } | ||
| DRInitializePragma >> scope [ | ||
|
|
||
| ^ scope | ||
| ] | ||
|
|
||
| { #category : 'accessing' } | ||
| DRInitializePragma >> scope: anObject [ | ||
|
|
||
| scope := anObject | ||
| ] |
There was a problem hiding this comment.
The scope of the pragma is always the method scope. The instructions already knows their CFG
self controlFlowGraph scope
| DRBytecodeGenerator >> visitPragmaNode: aPragmaNode [ | ||
|
|
||
| builder addPragma: aPragmaNode pragma. | ||
| ] |
There was a problem hiding this comment.
Ahh ok, this is for adding it to the metadata in the literals
| { #category : 'initialization' } | ||
| DRMethodIRGenerator >> initializePragma: pragmas [ | ||
|
|
||
| | instr | | ||
| instr := self currentBasicBlock initializePragma. | ||
| instr pragmas: pragmas. | ||
| instr scope: self scope. | ||
| pragmas do: [ :each | each acceptVisitor: self]. | ||
| ] |
There was a problem hiding this comment.
Would not be better to have one instructions per pragma? DRPragmas instead of Init...?
| DRMethodIRGenerator >> interpretCode: aDRMethod receiver: aReceiver [ | ||
|
|
||
| self setupCFGScope: aDRMethod codeNode. | ||
| self initializePragma: aDRMethod codeNode pragmas. |
There was a problem hiding this comment.
I think that the CFG should only have a InitPragma instruction if there are pragmas in the method, and not always.
Also, here we manage any pragma, not only primitives... I don't know if that is good or bad, just saying 😇
| { #category : 'visiting' } | ||
| DRMethodIRGenerator >> visitPragmaNode: aPragmaNode [ | ||
|
|
||
| | var varNode| | ||
| "if the pragma defines an error variable, we need to declare a temp and add it to scope" | ||
|
|
||
| aPragmaNode isPrimitiveErrorPragma ifFalse: [ ^ self ]. | ||
| varNode := OCVariableNode named: aPragmaNode primitiveErrorVariableName. | ||
| var := PrimitiveErrorVariable node: varNode. | ||
| self ir scope addTempVariables: { var } | ||
|
|
||
| ] |
There was a problem hiding this comment.
Ohhh our scopes have AST nodes as variables, I didn't remember that :P
There was a problem hiding this comment.
We can open an issue for this
|
Ahhh and I don't understand if the error variable is working or not, because you said not but there are changes related to that. If necessary, we can move that to another issue, and optimize the temp vectors before. |
2 participants
Fix: #243
Now, can compile successfully pragma with only simple primitive (eg: primitive:1)
For the case of pragma that defines an error variable, we need to define a temp for it. But for now, we are always using temp vector for handling all the temps (in the picture). So it breaks the execution of the method.
We need to rework on it after the optimisation of the use of the temp vector.