Implement `goto` in LPython #1163

ronnuriel · 2022-09-28T13:25:35Z

No description provided.

czgdp1807 · 2022-09-29T10:12:29Z

src/runtime/ltypes/goto.py

+
+
+def _make_code(code, codestring):
+    return code.replace(co_code=codestring)


Instead of manually calling types.Code(*args) as in the original source code I am using .replace method and passing the updated code. This makes it work with Python > 3.8 robustly since we don't depend on the internal order of arguments to PyCode_New.

czgdp1807 · 2022-09-29T10:14:17Z

@certik This is ready and works for CPython, LLVM and C backend. The file src/runtime/ltypes/goto.py is taken from snoack/python-goto. I have the following questions,

How should I give credit to python-goto developers? They use UNLICENSE which doesn't impose any legal restrictions on us. Should I add the commit we are using in a comment at the top of src/runtime/ltypes/goto.py (our goto.py file)?
I figured out a fix (see Implement goto in LPython #1163 (comment)). My question is should I send a PR to the original project or is it not necessary because we are not facing the burden of supporting very old Python versions.

Please let me know. Other than the above questions goto feature is fully ready from my side.

rebcabin · 2022-09-29T14:02:32Z

Is "UNLICENSE" equivalent to "PUBLIC DOMAIN?" If we have any other "derivative works" in our sources, how are we giving credit to the authors of the originals?

czgdp1807 · 2022-09-29T14:03:43Z

Is "UNLICENSE" equivalent to "PUBLIC DOMAIN?"

Correct as far as I can tell.

If we have any other "derivative works" in our sources, how are we giving credit to the authors of the originals?

I don't think we have any AFAICT. @certik Can you please confirm.

rebcabin · 2022-09-29T14:07:02Z

I figured out a fix (see Goto #1163 (comment)). My question is should I send a PR to the original project or is it not necessary because we are not facing the burden of supporting very old Python versions.

I would say we do not need the original project to pull any of our changes because we will never support earlier versions of Python. Is there any residual benefit to them? What's our version floor? 3.7, 3.9, ... ?

rebcabin · 2022-09-29T14:07:48Z

3. I figured out a fix (see Goto #1163 (comment)). My question is should I send a PR to the original project or is it not necessary because we are not facing the burden of supporting very old Python versions.

I would say we do not need the original project to pull any of our changes because we will never support earlier versions of Python. Is there any residual benefit to them? What's our version floor? 3.7, 3.9, ... ?

Looks like we're 3.9 and greater for LPython.

certik · 2022-09-29T22:04:24Z

src/libasr/ASR.asdl

@@ -173,10 +173,10 @@ stmt
        -- GoTo points to a GoToTarget with the corresponding target_id within
        -- the same procedure. We currently use `int` IDs to link GoTo with
        -- GoToTarget to avoid issues with serialization.
-    | GoTo(int target_id)
+    | GoTo(int target_id, identifier name)


I think GoTo should just reference using an ID. The name should go into GoToTarget.

Suggested change

| GoTo(int target_id, identifier name)

| GoTo(int target_id)

Or am I missing something?

Well in C backend we need the name to produce, label name_of_the_label or goto name_of_the_label. So we need the name to be stored in both GoTo and GoToTarget.

certik · 2022-09-29T22:08:10Z

src/lpython/semantics/python_ast_to_asr.cpp

+        if( !ASR::is_a<ASR::stmt_t>(*tmp) ) {
+            ASRUtils::EXPR(tmp);
+            tmp = nullptr;
+        }


What is the purpose of these lines, both the original and the new ones?

Are we checking that the result is an expression? If so, you can use an assert for ASR::is_a (or perhaps down_cast, since is_a might require a particular class).

So the scene is that goto.end is represented as [(Expr (Attribute (Name goto Load) end Load))] at AST level. So, now when we will complete visiting Attribute, tmp will either point to a ASR expression or a ASR statement (because GoTo in ASR is a statement). So in case we get something like, goto.label then tmp will point to a statement in which case we should exit the function naturally and the transform_stmts before this node will include that GoTo statement. However if tmp is not pointing to a statement then I replaced ASRUtils::EXPR(tmp) with an LFORTRAN_ASSERT.

If so, you can use an assert for ASR::is_a (or perhaps down_cast, since is_a might require a particular class).

Assert with ASR::is_a works fine.

Can you please document this as a comment? Next person to see that code will have the same question, including me tomorrow.

certik

I think this is good enough to merge as is. But there are issues to resolve. One is the ASR relationship of GoTo and GoToTarget. It looks like what is needed is a direct link (in C++) from goto to the target, so that one can read off the name if needed easily.

It seems the ASR.asdl structure does not easily allow a way to do it. Storing both the number and a string is redundant.

certik · 2022-09-29T22:15:35Z

Regarding where to maintain the CPython implementation, yes, we should maintain it ourselves and ensure it actually works in all modern Python versions.

The source code of src/runtime/ltypes/goto.py is coming from https://github.com/snoack/python-goto/blob/acbe736221d2238df3d09beab457d0bb19d05812/goto.py. The license of the python-goto project at the time of inclusion in LPython is available at https://github.com/snoack/python-goto/blob/acbe736221d2238df3d09beab457d0bb19d05812/LICENSE.

Co-authored-by: Ron Nuriel <[email protected]>

czgdp1807 · 2022-09-30T03:12:02Z

It looks like what is needed is a direct link (in C++) from goto to the target, so that one can read off the name if needed easily.

It seems the ASR.asdl structure does not easily allow a way to do it. Storing both the number and a string is redundant.

There are two ways in my mind to deal with this,

Store only the name and generate IDs in the backend. This goes against the principle that we backend shouldn't need to track anything.
Change GoTo(int target_id, identifier name) to GoTo(stmt_t goto_target). Then one can do ASR::down_cast<ASR::GoToTarget>(goto_object->goto_target)->m_name. One extra indirection but it should removes the redundancy.

certik · 2022-09-30T21:07:51Z

Change GoTo(int target_id, identifier name) to GoTo(stmt_t goto_target). Then one can do ASR::down_cast<ASR::GoToTarget>(goto_object->goto_target)->m_name. One extra indirection but it should removes the redundancy.

I like this idea, so I opened up a dedicated issue for this: #1167

ronnuriel requested a review from czgdp1807 September 28, 2022 13:25

ronnuriel self-assigned this Sep 28, 2022

czgdp1807 force-pushed the goto branch from 12bb000 to d921cf6 Compare September 28, 2022 13:30

ronnuriel requested a review from certik September 28, 2022 14:03

ronnuriel added c Label for C language related changes asr ASR related changes labels Sep 28, 2022

czgdp1807 marked this pull request as draft September 29, 2022 02:56

czgdp1807 marked this pull request as ready for review September 29, 2022 08:19

czgdp1807 force-pushed the goto branch from 225e958 to dc196bc Compare September 29, 2022 10:00

czgdp1807 reviewed Sep 29, 2022

View reviewed changes

czgdp1807 changed the title ~~Goto~~ Implement goto in LPython Sep 29, 2022

certik reviewed Sep 29, 2022

View reviewed changes

certik approved these changes Sep 29, 2022

View reviewed changes

czgdp1807 force-pushed the goto branch from 2ea67a4 to 655b023 Compare September 30, 2022 02:53

czgdp1807 and others added 7 commits September 30, 2022 08:26

Added test cases for goto statement

cc9bf94

Co-authored-by: Ron Nuriel <[email protected]>

Transform goto and label in AST to GoTo and GoToTarget in ASR

a0991ec

Support GoTo and GoToTarget in C backend

9e938f7

Co-authored-by: Ron Nuriel <[email protected]>

Add name of the label in GoTo and GoToTarget

fa57b76

Update call to make_GoTo_t constructor

3f8111a

Register tests for goto statement

3a5f0f5

Updated reference tests

d5b21d6

czgdp1807 force-pushed the goto branch from 655b023 to d5b21d6 Compare September 30, 2022 02:57

czgdp1807 enabled auto-merge September 30, 2022 02:57

czgdp1807 merged commit 5a99a6a into lcompilers:main Sep 30, 2022

czgdp1807 deleted the goto branch September 30, 2022 03:20

certik mentioned this pull request Sep 30, 2022

Pointers / links in ASDL #1167

Open



		def _make_code(code, codestring):
		return code.replace(co_code=codestring)

Implement goto in LPython #1163

Implement goto in LPython #1163

Uh oh!

Conversation

ronnuriel commented Sep 28, 2022

Uh oh!

czgdp1807 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czgdp1807 commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebcabin commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czgdp1807 commented Sep 29, 2022

Uh oh!

rebcabin commented Sep 29, 2022

Uh oh!

rebcabin commented Sep 29, 2022

Uh oh!

certik Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

czgdp1807 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

czgdp1807 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

czgdp1807 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

certik Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

certik commented Sep 29, 2022

Uh oh!

czgdp1807 commented Sep 30, 2022

Uh oh!

certik commented Sep 30, 2022

Uh oh!

Uh oh!

Implement `goto` in LPython #1163

Implement `goto` in LPython #1163

czgdp1807 Sep 29, 2022 •

edited

Loading

czgdp1807 commented Sep 29, 2022 •

edited

Loading

rebcabin commented Sep 29, 2022 •

edited

Loading

czgdp1807 Sep 29, 2022 •

edited

Loading