-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Move variable slotting to the IR tree #51452
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a couple minor suggestions
import org.objectweb.asm.Label; | ||
import org.objectweb.asm.Opcodes; | ||
|
||
public class DoWhileLoopNode extends LoopNode { | ||
|
||
public DoWhileLoopNode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need an explicit empty ctor the same as the default ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Missed this during the merge.
methodWriter.visitVarInsn(MethodWriter.getType(variable.clazz).getOpcode(Opcodes.ISTORE), variable.getSlot()); | ||
methodWriter.visitVarInsn(variable.getAsmType().getOpcode(Opcodes.ISTORE), variable.getSlot()); | ||
|
||
Variable loop = scopeTable.getVariable("#loop"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have another method for special/private/hidden (whatever we want to call them) variables? I'd rather have a dedicated method than have to remember #
is outside our grammar and so this cannot collide with user variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added defineInternalVariable and getInternalVariable to address this specific issue. Let me know if you'd prefer a different name, but I like "internal" because I think it makes it clear this is something defined by the compiler rather than a user.
@rjernst Thanks for the review! |
@stu-elastic I'd like to continue to make progress on this, so I'm going to go ahead and merge with the approval from @rjernst. When you have more time, I'm happy to go over the code here and make follow up PR changes if necessary. |
Currently, variable slotting (a variable's index on the ASM stack) is generated by the user tree during semantic checking. With the split it no longer makes sense to have the user tree generate a variable's slot. This PR moves variable slot allocation to the IR tree using the class ScopeTable. This class holds variables currently in scope and calculates an appropriate slot for them. This also allows for optimization phases to occur on the IR tree because now variables can be injected without having to update slotting.
For review the most important class to look at in this PR is the newly added ScopeTable. The majority of this PR is boilerplate adding ScopeTable to the IR nodes write method signature.