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

Fixed output through c_sym backend for test_gruntz.py #2441

Closed
wants to merge 2 commits into from

Conversation

anutosh491
Copy link
Collaborator

As discussed in #2437 (comment), this fixes the issue with the c_sym backend to correctly deal with test_gruntz.py (and any symbolic list in general)

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat integration_tests/test_gruntz.py 
from lpython import S
from sympy import Symbol

def mmrv(e: S, x: S) -> list[S]:
    l: list[S] = []
    if not e.has(x):
        return l
    else:
        raise

def test_mrv1():
    x: S = Symbol("x")
    y: S = Symbol("y")
    ans: list[S] = mmrv(y, x)
    assert len(ans) == 0
    print(ans)

test_mrv1()(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine --backend=c integration_tests/test_gruntz.py 
[]

@@ -529,7 +524,12 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
} else if (ASR::is_a<ASR::List_t>(*v_m_type)) {
ASR::List_t* t = ASR::down_cast<ASR::List_t>(v_m_type);
std::string list_type_c = c_ds_api->get_list_type(t);
sub = format_type_c("", list_type_c, v.m_name,
std::string name = v.m_name;
if (name == "_lpython_return_variable" && v.m_intent == ASRUtils::intent_out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is hardwiring a check for _lpython_return_variable at several places in the C backend.

However, the C backend should work with any frontend, and should be independent of how the frontend names its variables.

Is there a way to fix it without any such checks of the type name == "_lpython_return_variable"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall look into it !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are a couple places to consider if you think about it

  1. Whenever use use the subroutine_from_function, we would end up with this ASR node
        (=
            (Var 3 _lpython_return_variable)
            (Var 3 some_var)
            ()
        )

As we are finally storing whatever answer we want to return in the variable of intent Out (which in our case and I think most cases would be _lpython_return_variable or basically the variable to be returned.)
So I think we could do with just the m_intent == ASRUtils::intent_out check for any normal case as we know the return_var is changed to out. Only when some exceptional/corner case comes up we might have to think about this

The "_lpython_return_variable" just adds another level of security and in general gives more protection.

Copy link
Collaborator Author

@anutosh491 anutosh491 Dec 13, 2023

Choose a reason for hiding this comment

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

I would say we can just use it once in the beggining (when we hit the variable)

  mmrv:
      (Function
          (SymbolTable
              3
              {
                  _lpython_return_variable:
                      (Variable
                          3
                          _lpython_return_variable
                          []
                          Out
                          ()
                          ()
                          Default
                          (List
                              (CPtr)
                          )
                          ()
                          Source
                          Public
                          Required
                          .false.
                      ),

Here we can check

              if (name == "_lpython_return_variable" && v.m_intent == ASRUtils::intent_out) {
                  is_return_var_intent_out = true;

Once this is done we can always use the bool variable is_return_var_intent_out for anything we want.
If we don't want to do this, we can obviously make it a bit more specific to cater to cases relevant to us for now

              if (t->m_type->type == ASR::ttypeType::CPtr && v.m_intent == ASRUtils::intent_out) {
                  is_return_var_intent_out = true;
              }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After some thought and checking for some corner cases I've realized that we might be able to do without using "_lpython_return_variable" but it's best (in terms of convinience) to use it. Let's discuss more in the call today.

@@ -529,7 +524,12 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
} else if (ASR::is_a<ASR::List_t>(*v_m_type)) {
ASR::List_t* t = ASR::down_cast<ASR::List_t>(v_m_type);
std::string list_type_c = c_ds_api->get_list_type(t);
sub = format_type_c("", list_type_c, v.m_name,
std::string name = v.m_name;
if (name == "_lpython_return_variable" && v.m_intent == ASRUtils::intent_out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The fix might be as simple as:

Suggested change
if (name == "_lpython_return_variable" && v.m_intent == ASRUtils::intent_out) {
if (v.m_intent == ASRUtils::intent_out) {

If it doesn't work, then let's figure out an example where this fails and understand if there are other corner cases to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the condition is that if it is a List, it should be passes as a pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants