Added IntrinsicFunction in grammar #1616

anutosh491 · 2023-03-24T07:24:03Z

As I've been working on #1607 (comment) , which requires LPython to support IntrinsicFunction as expr . It seems it would also help us out at other places like #1606 (review) .

Thirumalai-Shaktivel · 2023-03-24T13:16:13Z

@anutosh491 do you need any help in completing this PR?

I think, these PR changes have to be merged:

czgdp1807 · 2023-03-24T13:26:32Z

@Thirumalai-Shaktivel Please go ahead and push all the necessary changes here. Please add LPython tests also if necessary.

Thirumalai-Shaktivel · 2023-03-24T15:05:38Z

I will try to complete this tomorrow. @anutosh491 thanks for opening a PR for this.

anutosh491 · 2023-03-25T02:29:18Z

I will try to complete this tomorrow. @anutosh491 thanks for opening a PR for this.

Sure, would be a good addition to have here !

Thirumalai-Shaktivel · 2023-03-26T13:11:07Z

I implemented IntrinsicFunction for the built-in abs.
We create the IntrinsicFunction in AST->ASR and intrinsic_function pass we import the function from lpython_builtin.py and use that function to compute the value.

Review this commit: 80c59fd and bc505b3

What are your thoughts on this? @certik @czgdp1807 @Smit-create

anutosh491 · 2023-03-27T03:19:08Z

I am just curious about a few things , which I would like to point out here .

If we generate the ASR for something like the following

def main0():
    print(sin(0.5))

main0()

It goes like this


                                        [(IntrinsicFunction
                                             Sin
                                            [(RealConstant
                                                0.500000
                                                (Real 8 [])
                                            )]
                                            0
                                            (Real 8 [])
                                            (RealConstant
                                                0.479426
                                                (Real 8 [])
                                            )
                                        )]

Here do we expect Sin to be displayed too ? I mean I just thought we would need the intrinsic_id to be displayed according to IntrinsicFunction(int intrinsic_id, expr* args, int overload_id, ttype type, expr? value)

anutosh491 · 2023-03-27T03:22:10Z

Also something like the following gives an error

def main0():
    sin(0.5)     # whereas calling through the print function works !

main0()

or just

sin(0.5)

gives .

(lf) anutosh491@spbhat68:~/lpython/lpython$ ./src/bin/lpython --show-asr examples/expr2.py --indent
semantic error: Function 'sin' is not declared
 --> examples/expr2.py:2:5
  |
2 |     sin(0.5)
  |     ^^^^^^^^

It's counterpart, LFortran works perfectly in this case !

Thirumalai-Shaktivel · 2023-03-27T04:06:01Z

Here do we expect Sin to be displayed too ?

Yup, intrinsic_id is converted to display Sin
See:

https://github.com/lfortran/lfortran/blob/36f604cce47f879594b3f1852274409257850222/src/libasr/pass/intrinsic_function_registry.h#L298-L313

Intrinsic_id is just an integral value, But I think Sin makes more sense instead of just a number.

Thirumalai-Shaktivel · 2023-03-27T04:11:05Z

#1616 (comment)

I'm not sure about this, but I think the intrinsicFunction is designed for built-in function. So, functions like abs, range, list, ... can be represented in intrinsicfunction. Whereas, sin is imported from the math Module (from math import sin). So, I think there is no need for intrinsicfunction.

@certik @czgdp1807 Am I correct?

Thirumalai-Shaktivel · 2023-03-27T05:57:18Z

def main0():
   sin(0.5)     # whereas calling through the print function works !

main0()

print(sin(0.5)), I think this should not work, if it works then we have to raise an error.

$ python examples/expr2.py
Traceback (most recent call last):
  File "/Users/thirumalai/Open_Source/lpython/examples/expr2.py", line 4, in <module>
    main0()
  File "/Users/thirumalai/Open_Source/lpython/examples/expr2.py", line 2, in main0
    print(sin(0.5))
NameError: name 'sin' is not defined. Did you mean: 'bin'?

anutosh491 · 2023-03-27T16:01:37Z

Ohhh so print(sin(0.5)) doesn't work for you , works for me though !

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py
def main0():
    print(sin(0.5))

main0()

(lf) anutosh491@spbhat68:~/lpython/lpython$ ./src/bin/lpython --show-asr examples/expr2.py
(TranslationUnit (SymbolTable 1 {_global_symbols: (Module (SymbolTable 108 {_lpython_main_program: (Function (SymbolTable 107 {}) _lpython_main_program (FunctionType [] () Source Implementation () .false. .false. .false. .false. .false. [] [] .false.) [main0] [] [(SubroutineCall 108 main0 () [] ())] () Public .false. .false.), main0: (Function (SymbolTable 2 {}) main0 (FunctionType [] () Source Implementation () .false. .false. .false. .false. .false. [] [] .false.) [] [] [(Print () [(IntrinsicFunction Sin [(RealConstant 0.500000 (Real 8 []))] 0 (Real 8 []) (RealConstant 0.479426 (Real 8 [])))] () ())] () Public .false. .false.)}) _global_symbols [] .false. .false.), lpython_builtin: (IntrinsicModule lpython_builtin), main_program: (Program (SymbolTable 106 {_lpython_main_program: (ExternalSymbol 106 _lpython_main_program 108 _lpython_main_program _global_symbols [] _lpython_main_program Public)}) main_program [_global_symbols] [(SubroutineCall 106 _lpython_main_program () [] ())])}) [])

(lf) anutosh491@spbhat68:~/lpython/lpython$ ./src/bin/lpython examples/expr2.py
4.79425538604203005e-01

Edit : Ahh my bad , I realize that's the error which we should be returning , what you've pointed out !

certik · 2023-03-27T20:07:48Z

I think we have to intercept math.sin (for just scalars) as well as numpy.sin (for both scalars and arrays) and create IntrinsicFunction for them.

certik · 2023-03-27T20:10:37Z

I see, but because currently the math ones are defined in our LPython runtime library in Python, then we can leave those I guess for now, and port them to IntrinsicFunction later.

certik · 2023-03-29T15:49:48Z

src/libasr/pass/intrinsic_function_registry.h

+    };
+
+    static inline bool is_intrinsic_function(const std::string& name) {
+        return intrinsic_function_by_name_db.find(name) != intrinsic_function_by_name_db.end();


I would put a check for "sin" here, and ignore everything else for now, and use existing LPython mechanism for those. Let's get "sin" working in this PR, merge it. Then we can add more functions, one by one.

certik · 2023-03-29T15:50:34Z

src/lpython/semantics/python_ast_to_asr.cpp

@@ -6106,7 +6123,20 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> {
        }

        if (!s) {
-            if (intrinsic_procedures.is_intrinsic(call_name)) {
+            if (ASRUtils::IntrinsicFunctionRegistry::is_intrinsic_function(call_name)) {


Just do it here instead, that way libasr stays the same.

"If sin, then something, otherwise original."

certik · 2023-03-29T15:54:25Z

LPython doesn't have sin yet, so I would do "abs", as you did, but let's do it in LFortran first, get everything working there, that should be relatively easy. Then port it here, and use it here.

Thirumalai-Shaktivel · 2023-04-02T04:54:32Z

I'm currently working on this: #1616 (comment). After completing that I will finish this PR.
I will try to complete everything by today.

czgdp1807 · 2023-04-02T04:57:11Z

Great. After that I will do the sync (on Tuesday).

certik · 2023-04-03T18:08:23Z

@Thirumalai-Shaktivel so abs (lfortran/lfortran#1480) is in. This PR can now be finished.

Thirumalai-Shaktivel · 2023-04-04T04:25:19Z

Almost completed, @czgdp1807 after that you can review and merge this.

Thirumalai-Shaktivel · 2023-04-04T14:00:35Z

This PR is ready!
@certik @czgdp1807 kindly review and share your feedback.

There are some changes in libasr here, I will submit another PR for it there.

czgdp1807 · 2023-04-04T16:52:24Z

There are some changes in libasr here, I will submit another PR for it there.

Please do it right away. We will merge both PRs together. I hope they aren't big changes?

Thirumalai-Shaktivel · 2023-04-04T17:20:36Z

Thanks, for the review.
I created a PR in LFortran: lfortran/lfortran#1493

certik

I think that this is fine.

Co-authored-by: Thirumalai-Shaktivel <thirumalaishaktivel@gmail.com>

anutosh491 marked this pull request as draft March 24, 2023 07:24

Thirumalai-Shaktivel self-assigned this Mar 24, 2023

certik mentioned this pull request Mar 25, 2023

New string Method endswith() support #1606

Merged

Thirumalai-Shaktivel force-pushed the Add_Intrinsic branch from ce0b35b to 81360d6 Compare March 26, 2023 14:02

anutosh491 and others added 5 commits March 26, 2023 19:33

Added IntrinsicFunction in grammar

b3818fe

XX Sync from LFortran

ade4113

Implement Abs builtin function

2dfae00

Fix the return type for abs

bc505b3

Update tests

4783466

Thirumalai-Shaktivel force-pushed the Add_Intrinsic branch 2 times, most recently from dd498f9 to 4783466 Compare March 26, 2023 14:07

certik reviewed Mar 29, 2023

View reviewed changes

czgdp1807 mentioned this pull request Apr 2, 2023

ASR: Sync Libasr from LFortran #1658

Closed

czgdp1807 mentioned this pull request Apr 3, 2023

ASR: Sync Libasr from LPython lfortran/lfortran#1478

Closed

Thirumalai-Shaktivel added 2 commits April 4, 2023 09:30

Sync abs changes from LFortran

6920f6d

Add a check for function not available in CPython builtin

cc7d331

Thirumalai-Shaktivel added 8 commits April 4, 2023 10:04

Merge branch 'main' into Add_Intrinsic

ed65b0c

Use integer cast for logical types

54e90c9

Revert expr2.py

c4bc689

Alter the intrinsic Function name mangling

aafc552

Create the intrinsics Function in the unit.global_scope

3ff0f84

Use IntrinsicFunction pass in ASR->CPP

b60c3e0

Visit global_func first to define all the functions

df34228

Use IntrinsicFunction pass in ASR->WASM

3391a44

Thirumalai-Shaktivel marked this pull request as ready for review April 4, 2023 15:49

czgdp1807 approved these changes Apr 4, 2023

View reviewed changes

certik approved these changes Apr 5, 2023

View reviewed changes

Merge main into Add_intrinsic

35955ac

czgdp1807 enabled auto-merge (squash) April 5, 2023 12:32

czgdp1807 merged commit b35e744 into lcompilers:main Apr 5, 2023

virendrakabra14 pushed a commit to virendrakabra14/lpython that referenced this pull request Apr 8, 2023

Added IntrinsicFunction in ASR (lcompilers#1616)

c26a3df

Co-authored-by: Thirumalai-Shaktivel <thirumalaishaktivel@gmail.com>

anutosh491 deleted the Add_Intrinsic branch May 20, 2023 04:30

Added IntrinsicFunction in grammar #1616

Added IntrinsicFunction in grammar #1616

Uh oh!

Conversation

anutosh491 commented Mar 24, 2023

Uh oh!

Thirumalai-Shaktivel commented Mar 24, 2023

Uh oh!

czgdp1807 commented Mar 24, 2023

Uh oh!

Thirumalai-Shaktivel commented Mar 24, 2023

Uh oh!

anutosh491 commented Mar 25, 2023

Uh oh!

Thirumalai-Shaktivel commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented Mar 27, 2023

Uh oh!

anutosh491 commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Mar 27, 2023

Uh oh!

anutosh491 commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Mar 27, 2023

Uh oh!

certik commented Mar 27, 2023

Uh oh!

certik Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

certik Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

certik commented Mar 29, 2023

Uh oh!

Thirumalai-Shaktivel commented Apr 2, 2023

Uh oh!

czgdp1807 commented Apr 2, 2023

Uh oh!

certik commented Apr 3, 2023

Uh oh!

Thirumalai-Shaktivel commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Apr 4, 2023

Uh oh!

czgdp1807 commented Apr 4, 2023

Uh oh!

Thirumalai-Shaktivel commented Apr 4, 2023

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Mar 26, 2023 •

edited

Loading

anutosh491 commented Mar 27, 2023 •

edited

Loading

Thirumalai-Shaktivel commented Mar 27, 2023 •

edited

Loading

Thirumalai-Shaktivel commented Mar 27, 2023 •

edited

Loading

anutosh491 commented Mar 27, 2023 •

edited

Loading

Thirumalai-Shaktivel commented Apr 4, 2023 •

edited

Loading