Skip to content

Comments

Added support for functions to accept symbolic variables#2331

Merged
certik merged 5 commits intolcompilers:mainfrom
anutosh491:Fixing_symbolic_parameters
Sep 24, 2023
Merged

Added support for functions to accept symbolic variables#2331
certik merged 5 commits intolcompilers:mainfrom
anutosh491:Fixing_symbolic_parameters

Conversation

@anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Sep 20, 2023

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

anutosh491 commented Sep 20, 2023

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

anutosh491 commented Sep 20, 2023

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

anutosh491 commented Sep 21, 2023

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 •
edited
Loading

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

Thirumalai-Shaktivel commented Sep 22, 2023

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 •
edited
Loading

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 anutosh491 requested a review from certik September 23, 2023 06:04
@anutosh491 anutosh491 mentioned this pull request Sep 23, 2023
Merged
@anutosh491 anutosh491 marked this pull request as draft September 23, 2023 11:58
@anutosh491
Copy link
Collaborator Author

anutosh491 commented Sep 23, 2023 •
edited
Loading

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

anutosh491 commented Sep 24, 2023

I think this is ready now.

certik
certik approved these changes Sep 24, 2023
View reviewed changes
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
@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

Reviewers

@certik certik certik approved these changes

Assignees

No one assigned

Labels

symbolic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anutosh491 @certik @Thirumalai-Shaktivel