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

Implemented Lshift #2556

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Implemented Lshift #2556

wants to merge 3 commits into from

Conversation

Kishan-Ved
Copy link
Contributor

I thought this would be a good one to get started with LPython implementations.

Reference: https://docs.python.org/3/library/operator.html#operator.lshift

@Kishan-Ved
Copy link
Contributor Author

Kishan-Ved commented Feb 27, 2024

@certik Sir the error seems weird. Can we rerun this?

Also, I noticed that many intrinsic functions like Trailz don't have integration tests.

Edit: I have fixed the windows CI error in #2562

@@ -816,6 +816,7 @@ RUN(NAME callback_04 IMPORT_PATH .. LABELS cpython)
# Intrinsic Functions
RUN(NAME intrinsics_01 LABELS cpython llvm NOFAST) # any
RUN(NAME intrinsics_02 LABELS cpython llvm c) # floordiv
RUN(NAME intrinsics_03 LABELS llvm) # lshift
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
RUN(NAME intrinsics_03 LABELS llvm) # lshift
RUN(NAME intrinsics_03 LABELS cpython llvm) # lshift

This verifies that the test works with python as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check the comment below: #2556 (comment)

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

Looks good, I have left some comments.

src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
@Thirumalai-Shaktivel
Copy link
Collaborator

Please mark this PR ready for review once it is ready.

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as draft March 7, 2024 05:43
@Kishan-Ved
Copy link
Contributor Author

Kishan-Ved commented Mar 7, 2024

The test fails for cpython, may I please know the reason. The error is:

Traceback (most recent call last):
  File "/home/kishan/Desktop/lpython/integration_tests/intrinsics_03.py", line 8, in <module>
    print(lshift(a,b))
NameError: name 'lshift' is not defined

The file runs when I do ./src/bin/lpython integration_tests/intrinsics_03.py and gives the correct output.

Do I need to make more additions to some files?

@Thirumalai-Shaktivel
Copy link
Collaborator

Thirumalai-Shaktivel commented Mar 9, 2024

It fails because, python doesn't have builtin lshift support. I think it needs to be imported from the operator module, see: https://docs.python.org/3/library/operator.html#operator.lshift. Something like:

from operator import lshift
print(lshift(x, y))

Does x << y work in lpython?

@Kishan-Ved
Copy link
Contributor Author

It fails because, python doesn't have builtin lshift support. I think it needs to be imported from the operator module, see: https://docs.python.org/3/library/operator.html#operator.lshift. Something like:

from operator import lshift
print(lshift(x, y))

Does x << y work in lpython?

Yes, x<<y works in lpython. Also, you are right, lshift() needs to be imported from the operator module and is not built in python.

@Kishan-Ved
Copy link
Contributor Author

Kishan-Ved commented Mar 9, 2024

It fails because, python doesn't have builtin lshift support. I think it needs to be imported from the operator module, see: https://docs.python.org/3/library/operator.html#operator.lshift. Something like:

from operator import lshift
print(lshift(x, y))

Does x << y work in lpython?

I want lshift to be imported only for the python test. For LPython's implementation, I want the control to be redirected to my intrinsic function. How can I achieve this?
Or should we include the cpython test later, when we implement the operator module for LPython, and there we redirect control to this intrinsic?

@Kishan-Ved
Copy link
Contributor Author

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

Successfully merging this pull request may close these issues.

2 participants