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

WIP: Labels and Macros #65

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

WIP: Labels and Macros #65

wants to merge 1 commit into from

Conversation

ssanderson
Copy link
Collaborator

No description provided.

@ssanderson ssanderson force-pushed the assembly branch 3 times, most recently from fb6e103 to 3bf5451 Compare March 7, 2018 06:11
Copy link
Collaborator Author

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

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

@llllllllll posted some notes for you here.

Label = instrs.Label


def assemble_function(signature, objs, code_kwargs=None, function_kwargs=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to decide how we want to manage signatures here.

Ideally it should be easy to either type this by hand or use the signature of an existing function. For the tests, I've been using inspect.signature(lambda <sig>: None) as an easy way to get the signature I want.

Copy link
Owner

Choose a reason for hiding this comment

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

one idea is to allow a function which would likely look like: lambda a, b, c: ...

instr._stolen_by = self
for jmp in instr._target_of:
jmp.arg = self
self._target_of = instr._target_of
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be an |=, but that can go in a separate PR.

Copy link
Owner

Choose a reason for hiding this comment

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

let's remember to do this

instrs.RETURN_VALUE(),
],
),
AssertFail("Shouldn't ever get here!"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is here because we currently can't compile a function that ends with just an IfStatement, because we emit a JUMP_FORWARD to a label after the else block.

One way to fix this might be to replace jumps to a final label with LOAD_CONST(None); RETURN_VALUE().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though even that would be wasteful in this case, since the JUMP_FORWARD instruction is after an unconditional return inside the if block. The thing we'd really want for this is a dead code elimination pass.

Copy link
Owner

Choose a reason for hiding this comment

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

We used to have an optimizer but it generated invalid code sometimes. We could always bring it back as an optional codetransformer.

return list(mapcat(_validate_instructions, objs))


def resolve_labels(objs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In an earlier draft of this I was allowing people to pass Macros as arguments to jumps, and I replaced jumps to macros with jumps to the first instruction here. Now that we have labels, I can't think of a use-case where passing a macro is a significant improvement over passing a label that's right before a label, though it does mean you have to make sure nothing gets put between your label and your macro. One thing we could consider is adding start and end labels to all macros and have __iter__ bookend the results of assemble with those.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that adding the start/end labels is easier to support elsewhere, but we can implement that later.

@@ -344,21 +344,18 @@ def __init__(self,
if kwarg is not None:
raise ValueError('cannot specify **kwargs more than once')
kwarg = argname[2:]
append_argname(argname)
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a test

def assemble(self):
raise NotImplementedError('assemble')

def __iter__(self):
Copy link
Owner

Choose a reason for hiding this comment

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

we decided to not support arbitrary iterators for now

return "{}({!r})".format(type(self).__name__, self.n)


class AssertFail(Macro):
Copy link
Owner

Choose a reason for hiding this comment

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

It might be nice to make a general Raise macro and have this just be short hand for Raise(AssertionError, message)`

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.

None yet

2 participants