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

feature/labelInLoop #479

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yi2255
Copy link
Contributor

@Yi2255 Yi2255 commented Dec 7, 2024

According to feedback from #467, I reimplemented the feature of the label statement under the context of loop structure. The design ideas are as follows:

  1. The operations BreakNested and ContinueNested are added. The two JS instructions both require a number input that represents the depth of the loop that the instruction wants to break or continue, and if the number exceeds the actual loop depth, then the actual loop depth is equal to depth %= actualLoopDepth.

  2. In JSLifter, any loop structure has a begin and end block. For example, beginWhileLoopBody. Each time it is loaded, actualLoopDepth is incremented, and the start position of this loop in the overall code string is recorded. When endWhileLoop is loaded, actualLoopDepth is decremented, and the recorded position for the current loop is popped.

  3. During the breakNested lifting process, the expected depth of the target loop to break is first determined using input % actualLoopDepth. Then, the starting position of the target loop is obtained from the recorded loop stack. Simultaneously, it is checked whether a label already exists at that position. If not, the label will be inserted, all subsequent loop starting positions are updated, and the position is marked as inserted. The continueNested process follows the same steps.

@Yi2255 Yi2255 mentioned this pull request Dec 7, 2024
Copy link
Collaborator

@saelo saelo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! First high-level round of feedback

override var opcode: Opcode { .breakNested(self) }

init() {
super.init(numInputs: 1, attributes: [.isJump], requiredContext: [.javascript, .loop])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the depth as input will be tricky, again because the mutator will change inputs around arbitrarily and you'll quickly get strings or objects or other stuff as depth. Instead, I'd recommend making the depth a property of this operation:

final class BreakNested: JsOperation {
    override var opcode: Opcode { .breakNested(self) }

    let depth: Int

    init(depth: Int) {
        ...
    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can then also let the OperationMutator mutate the depth property of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your analysis has been very helpful to me. Thank you so much! I will complete the revisions as soon as possible.

@@ -2064,6 +2064,22 @@ final class LoopContinue: JsOperation {
}
}

final class BreakNested: JsOperation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is my understanding correct that these can only be used inside loops? Or are they also usable outside of loops in certain circumstances?

If they must always be in a loop: maybe we should also call these LoopBreakNested or something like that to stay consistent with LoopBreak

If they can also appear elsewhere: should we drop the .loop context requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these can only be used in loops. This is an inappropriate naming issue, and I will later rename it to LoopBreakNested and LoopContinueNested.

@Yi2255 Yi2255 force-pushed the feature-labelInLoop branch from facb0f9 to 0702ab2 Compare December 14, 2024 12:34
@Yi2255 Yi2255 force-pushed the feature-labelInLoop branch from 0702ab2 to 0f003d9 Compare December 14, 2024 12:36
@Yi2255
Copy link
Contributor Author

Yi2255 commented Dec 14, 2024

Hi @saelo, I have made improvements in the following three aspects based on your insightful feedback:

  1. Rename label operation name to LoopBreakNested & LoopContinueNested;
  2. Update implement of LoopBreakNested & LoopContinueNested JsOperation, made the depth a property of these.
  3. Update operationMutator, mutate the depth property of these.

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.

2 participants