Printing top-level Expressions #2716

Vipul-Cariappa · 2024-05-24T14:17:36Z

Example:

❯ lp 
>>> i: i32 = 9
>>> i * i
81
>>> i ** i
387420480

I have rebased this branch of #2713.

Vipul-Cariappa · 2024-05-24T14:26:49Z

cc @Shaikh-Ubaid

Shaikh-Ubaid

Shared some comments below.

Shaikh-Ubaid · 2024-05-25T03:56:05Z

src/lpython/python_evaluator.cpp

+            result.type = EvalResult::none;
+        } else {
+            throw LCompilersException("FortranEvaluator::evaluate(): Return type not supported");
+        }


Did we previously add all the above code somewhere else? (It feels like we added it somewhere before).

If this is being duplicated then I think we should create a function out of it and call that function here.

I had initially implemented this in #2698, but we did not merge. That PR lacked tests. We also merged other changes between that PR and this. I was thinking of closing that PR without merging it once we merge this PR.

Shaikh-Ubaid · 2024-05-25T03:57:18Z

src/lpython/tests/test_llvm.cpp

@@ -608,7 +608,7 @@ define float @f()
    CHECK(std::abs(r - 8) < 1e-6);
 }

-TEST_CASE("PythonCompiler 1") {
+TEST_CASE("PythonCompiler i32 expressions") {


Let's add this as a separate test and not change the existing test.

Ok. I can do that.

Shaikh-Ubaid · 2024-05-25T04:01:32Z

src/lpython/semantics/python_ast_to_asr.cpp

+            if ((tmp) && (!ASR::is_a<ASR::expr_t>(*tmp))) {
+                LCOMPILERS_ASSERT(ASR::is_a<ASR::expr_t>(*tmp));
+                tmp = nullptr;


I did not quite get the logic here. It says if tmp exists and it is not an expression, then right inside the if, we have assert that tmp is an expression. tmp is not an expresssion in if and tmp is an expression in the assert are contradicting, right?

Yes. It does not make sense. I should remove that assertion.

Shaikh-Ubaid · 2024-05-25T04:02:28Z

Please mark as "Ready for review" when ready.

Shaikh-Ubaid · 2024-05-25T04:04:10Z

Also, I think we have some previous discussion about printing top-level expressions. Would you mind linking that PR here so that others can have a better context about this?

Vipul-Cariappa · 2024-05-25T05:01:40Z

Also, I think we have some previous discussion about printing top-level expressions. Would you mind linking that PR here so that others can have a better context about this?

We discussed this in #2698.
I had implemented some of this in that PR. That PR lacked tests. We also merged other changes between that PR and this. I was thinking of closing that PR without merging it once we merge this PR.

Shaikh-Ubaid · 2024-05-25T06:20:43Z

src/lpython/semantics/python_ast_to_asr.cpp

+            if ((tmp) && (!ASR::is_a<ASR::expr_t>(*tmp))) {
+                tmp = nullptr;
+            }


I am a bit confused here. We make tmp as nullptr everytime it is not an expression. But, why do we do so? I think this means, if tmp is a statement, then we are skipping it. Any specific reason to skip statements @Vipul-Cariappa ?

If tmp is an expression then we return it. So that we can print it. The pass_wrap_global_stmts function uses this tmp expression and returns the evaluated result of the expression for us to print it later using PythonCompiler::evaluate and interactive_python_repl.
If tmp is a statement, for example, an assignment x = 5 then we would not want to print anything, so we set tmp to nullptr.

x = 5 then we would not want to print anything, so we set tmp to nullptr

Although we do not want to print anything, but we do want x to be updated to 5. I am unsure, but if we skip the whole x = 5 statement, then would the value of x be updated to 5?

The pass_wrap_global_stmts function uses this tmp expression and returns the evaluated result of the expression for us

Does it actually return an evaluated result? Or does it simply convert expressions into assignment statements?

Yes. The statement will be executed and x will be updated to 5.

❯ lp >>> x: i32 >>> x 0 >>> x = 5 >>> x 5

Does it actually return an evaluated result? Or does it simply convert expressions into assignment statements?

It returns the evaluated result. You can run the interactive in verbose mode with -v flag to see the AST, ASR, and the LLVM IR generated. Doing so, you can see that it returns the result.

With the current approach, I doubt that we are/would-be skipping some stmts like empty() and c_p_pointer()

lpython/src/lpython/semantics/python_ast_to_asr.cpp

Lines 5220 to 5221 in 13bb54e

// This happens for c_p_pointer() and

// empty() (if target is of type allocatable)

See my comment below https://github.com/lcompilers/lpython/pull/2716/files#r1614616838 which I think would avoid this.

Vipul-Cariappa · 2024-05-25T07:03:33Z

src/lpython/tests/test_llvm.cpp

+    r = e.evaluate2("i: i32");
+    CHECK(r.ok);
+    CHECK(r.result.type == PythonCompiler::EvalResult::none);
+    r = e.evaluate2("i = 5");
+    CHECK(r.ok);
+    CHECK(r.result.type == PythonCompiler::EvalResult::statement);
+    r = e.evaluate2("i");
+    CHECK(r.ok);
+    CHECK(r.result.type == PythonCompiler::EvalResult::integer4);
+    CHECK(r.result.i32 == 5);


The test here makes sure that the execution of statements is not skipped.

Shaikh-Ubaid · 2024-05-25T12:04:21Z

src/lpython/semantics/python_ast_to_asr.cpp

+        if (eval_count > 0) {
+            // In Interactive mode
+            if ((tmp) && (!ASR::is_a<ASR::expr_t>(*tmp))) {
+                tmp = nullptr;
+            }
+        } else if (tmp && !ASR::is_a<ASR::stmt_t>(*tmp)) {


Can you check if doing the following works?

Suggested change

if (eval_count > 0) {

// In Interactive mode

if ((tmp) && (!ASR::is_a<ASR::expr_t>(*tmp))) {

tmp = nullptr;

}

} else if (tmp && !ASR::is_a<ASR::stmt_t>(*tmp)) {

if (eval_count == 0 && tmp && !ASR::is_a<ASR::stmt_t>(*tmp)) {

These changes work. I have committed it.

Co-authored-by: Shaikh Ubaid <[email protected]>

Shaikh-Ubaid

LGTM, Thanks! Nice work!

Vipul-Cariappa marked this pull request as ready for review May 24, 2024 14:26

Vipul-Cariappa added 3 commits May 25, 2024 09:21

printing top level expressions

81224d7

test cases for i32 and i64

4ff446a

fix typo

d7e9451

Shaikh-Ubaid force-pushed the global_stmts-pass branch from c384731 to d7e9451 Compare May 25, 2024 03:51

Shaikh-Ubaid reviewed May 25, 2024

View reviewed changes

Shaikh-Ubaid marked this pull request as draft May 25, 2024 04:02

update according to code review suggestions

22efacf

Vipul-Cariappa marked this pull request as ready for review May 25, 2024 05:05

Shaikh-Ubaid reviewed May 25, 2024

View reviewed changes

Vipul-Cariappa commented May 25, 2024

View reviewed changes

Shaikh-Ubaid reviewed May 25, 2024

View reviewed changes

Update src/lpython/semantics/python_ast_to_asr.cpp

49f46c6

Co-authored-by: Shaikh Ubaid <[email protected]>

Shaikh-Ubaid approved these changes May 26, 2024

View reviewed changes

Shaikh-Ubaid merged commit 2420448 into lcompilers:main May 26, 2024
14 checks passed

Vipul-Cariappa mentioned this pull request May 28, 2024

Interactive Shell: Printing top level expression #2698

Closed

3 tasks

Vipul-Cariappa deleted the global_stmts-pass branch May 28, 2024 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Printing top-level Expressions #2716

Printing top-level Expressions #2716

Vipul-Cariappa commented May 24, 2024

Vipul-Cariappa commented May 24, 2024

Shaikh-Ubaid left a comment

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid commented May 25, 2024

Shaikh-Ubaid commented May 25, 2024

Vipul-Cariappa commented May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 25, 2024

Shaikh-Ubaid May 25, 2024

Vipul-Cariappa May 26, 2024

Shaikh-Ubaid left a comment

	// This happens for c_p_pointer() and
	// empty() (if target is of type allocatable)

Printing top-level Expressions #2716

Printing top-level Expressions #2716

Conversation

Vipul-Cariappa commented May 24, 2024

Vipul-Cariappa commented May 24, 2024

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented May 25, 2024

Shaikh-Ubaid commented May 25, 2024

Vipul-Cariappa commented May 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment