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

Added support for functions to accept symbolic variables #2331

Merged
merged 5 commits into from
Sep 24, 2023

Conversation

anutosh491
Copy link
Collaborator

Now that we have started porting sympy functions to LPython, I realized that there are some errors while trying to have symbolic parameters. Consider the following program

from sympy import pi, Symbol

def printsym(x: S):
    print(x)

def main0():
    a : S = Symbol("x")
    printsym(a)

If we look at the C code generated for this.

void printsym(void* x)
{
    _x = 0;
    x = NULL;
    x = (void*) &_x;
    basic_new_stack(x);
    printf("%s\n", basic_str(x));
    basic_free_stack(x);
}

void main0()
{
    int64_t _a;
    void* a;
    _a = 0;
    a = NULL;
    a = (void*) &_a;
    basic_new_stack(a);
    symbol_set(a, "x");
    printsym(a);
    basic_free_stack(a);
}

But this would end up giving a segmentation fault, we don't need to apply the transformations which we apply on local variables (a in this case) to the parameterized variable (x in this case). Something as simple as the following would do the job.

void printsym(void* x)
{
    printf("%s\n", basic_str(x));
}

Hence the changes introduced are the following

  1. Only apply the transformation if a local variable is being considered. Taken care through an if check
  2. The parameter x: S is being changed to x: CPtr to replicate the void pointer (printsym(void* x))
  3. I don't think the parameterized variables need to be freed, only the local ones should be hence that is being taken care of .For eg
def printsym(x: S):
   a: S = pi
   print(a)
   print(x)

Here a should be freed but freeing of x has been taken care in main0

@anutosh491
Copy link
Collaborator Author

An example for reference

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from sympy import pi, Symbol

def printsym(x: S, y: S):
    z: S = Symbol("z")
    print(x + y + z + pi)

def main0():
    a : S = Symbol("x")
    b : S = Symbol("y")
    printsym(a, b)

if __name__ == "__main__":
   main0()
(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine --backend=llvm  examples/expr2.py 
x + y + z + pi
(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine --backend=c  examples/expr2.py 
x + y + z + pi

@certik
Copy link
Contributor

certik commented Sep 20, 2023

Can you add a test for this?

@anutosh491
Copy link
Collaborator Author

I think so maybe we would need a visit_SubRoutineCall function in our pass to handle something like this

from sympy import pi, Symbol
from lpython import S

def printsym(x: S):
    print(x)

def main0():
    printsym(Symbol("x"))

main0()

Where we are passing Symbol("x") directly rather than storing it in a symbolic variable first and then moving on from there.
I'll try to get this soon and add tests for the same .

@certik
Copy link
Contributor

certik commented Sep 20, 2023

Just add a test for the feature you implemented in this PR, and later once you implement printsym(Symbol("x")) you can test it too.

@anutosh491
Copy link
Collaborator Author

Yes, I think so I can implement the case for printsym(Symbol("x")) ( through a visit_SubroutineCall function) in the subsequent PR.

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Sep 21, 2023

Also I am not sure why do we get an error through the llvm backend. Everything looks correct to me as in I don't see any obvious error . Consider this simple program

from sympy import pi, Symbol

def main0():
    i: i32 = 2
    _i: S = S(i)
    print(_i)

The C backend works perfectly

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --show-c examples/expr2.py 

void main0()
{
    int64_t __i;
    void* _i;
    int32_t i;
    __i = 0;
    _i = NULL;
    _i = (void*) &__i;
    basic_new_stack(_i);
    i = 2;
    integer_set_si(_i, i);
    printf("%s\n", basic_str(_i));
    basic_free_stack(_i);
}

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine --backend=c examples/expr2.py 
2
(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --show-llvm examples/expr2.py 

declare void @_lpython_set_argv(i32, i8**)
code generation error: asr_to_llvm: module failed verification. Error:
Call parameter type does not match function signature!
  %3 = load i32, i32* %i, align 4
 i64  call void @integer_set_si(void* %2, i32 %3)

@Thirumalai-Shaktivel would you mind helping me out here. Once this is fixed the failing llvm_sym test woudl work as expected.

@Thirumalai-Shaktivel
Copy link
Collaborator

I think you have to cast the second argument from i32 to i64. As you can see, declare accepts i64.

declare void @integer_set_si(void*, i64)
...
  %3 = load i32, i32* %i, align 4
  call void @integer_set_si(void* %2, i32 %3)
...

In C, I think there might be an implicit cast for i32 to i64, so it works.

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Sep 23, 2023

I think you have to cast the second argument from i32 to i64. As you can see, declare accepts i64.

Oops, sorry I think so I overlooked this. Thanks for the help !
This is now ready.

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Sep 23, 2023

I have a few doubts regarding few corner cases, hence converting to draft.

@anutosh491 anutosh491 marked this pull request as ready for review September 24, 2023 08:53
@anutosh491
Copy link
Collaborator Author

I think this is ready now.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think that this is good. Thanks!

@certik certik merged commit f9b09dd into lcompilers:main Sep 24, 2023
12 checks passed
@anutosh491 anutosh491 deleted the Fixing_symbolic_parameters branch September 25, 2023 02:19
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.

3 participants