Skip to content

Fix ASR Slice frontend #1051

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.

Sign up for GitHub

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

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

Fix ASR Slice frontend #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3626b89
Do not consider slice object as item
Smit-create Aug 28, 2022
3562fc0
Make String Section backend inclusive for left and right limits.
Smit-create Aug 29, 2022
13f3b4b
Add and update tests
Smit-create Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integration_tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ RUN(NAME vec_01 LABELS cpython llvm)
RUN(NAME test_str_comparison LABELS cpython llvm c)
RUN(NAME test_bit_length LABELS cpython llvm c)
RUN(NAME str_to_list_cast LABELS cpython llvm)
RUN(NAME test_issue_332 LABELS llvm)

RUN(NAME generics_01 LABELS cpython llvm)
RUN(NAME generics_02 LABELS cpython llvm)
Expand Down
13 changes: 13 additions & 0 deletions integration_tests/test_issue_332.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from ltypes import i32

def test_issue_332():
arr: i32[4]
arr[0] = 1
arr[1] = 0
arr[2] = -4
print(arr[:2])
print(arr[1])
print(arr[2])
Comment on lines +8 to +10
Copy link
Copy Markdown
Collaborator

@czgdp1807 czgdp1807 Aug 29, 2022

Choose a reason for hiding this comment

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

Use assert for verifying that the outputs are actually correct?

Copy link
Copy Markdown
Collaborator Author

@Smit-create Smit-create Aug 29, 2022

Choose a reason for hiding this comment

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

I am not sure if we can assign Array Section at present (I think only string is supported).

    p: i32[4]
    arr[0] = 1
    arr[1] = 0
    arr[2] = -4
    p = arr[:2]

Throws this:

LFORTRAN_ASSERT(ASR::is_a<ASR::Character_t>(*ASRUtils::expr_type(x.m_v)) &&
AssertFailed: ASR::is_a<ASR::Character_t>(*ASRUtils::expr_type(x.m_v)) && n_dims == 0

Comment on lines +8 to +10
Copy link
Copy Markdown
Collaborator

@czgdp1807 czgdp1807 Oct 9, 2022

Choose a reason for hiding this comment

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

Usage of assert possible here to check if the outputs are actually correct?

Suggested change
print(arr[:2])
print(arr[1])
print(arr[2])
print(arr[:2])
print(arr[1])
print(arr[2])
arr_2: i32[2] = arr[:2]
assert arr_2[0] == 1
assert arr_2[1] == 0
assert arr[1] == 0
assert arr[2] == -4

Copy link
Copy Markdown
Collaborator Author

@Smit-create Smit-create Oct 10, 2022

Choose a reason for hiding this comment

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

Not yet:

File "/Users/thebigbool/repos/lpython/src/libasr/codegen/asr_to_llvm.cpp", line 1658
    LFORTRAN_ASSERT(ASR::is_a<ASR::Character_t>(*ASRUtils::expr_type(x.m_v)) &&
AssertFailed: ASR::is_a<ASR::Character_t>(*ASRUtils::expr_type(x.m_v)) && n_dims == 0

Copy link
Copy Markdown
Collaborator

@czgdp1807 czgdp1807 Oct 14, 2022

Choose a reason for hiding this comment

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

Makes sense. Can you try fixing the bug?

Copy link
Copy Markdown
Collaborator Author

@Smit-create Smit-create Oct 16, 2022

Choose a reason for hiding this comment

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

I think the LLVM backend currently only supports string-slicing assignments and not arrays because they can be n-dimensional. Is that intentional?



test_issue_332()
2 changes: 2 additions & 0 deletions src/libasr/runtime/lfortran_intrinsics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,11 @@ LFORTRAN_API char* _lfortran_str_copy(char* s, int32_t idx1, int32_t idx2) {
return dest_char;
}

// This functions considers idx1 and idx2 both inclusive.
LFORTRAN_API char* _lfortran_str_slice(char* s, int32_t idx1, int32_t idx2, int32_t step,
bool idx1_present, bool idx2_present) {
int s_len = strlen(s);
idx2++;
Copy link
Copy Markdown
Collaborator

@czgdp1807 czgdp1807 Aug 29, 2022

Choose a reason for hiding this comment

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

I am surprised why tests didn't fail after this change. Can you try applying this specific change to LFortran to make sure this works there as well.

Copy link
Copy Markdown
Collaborator Author

@Smit-create Smit-create Aug 29, 2022

Choose a reason for hiding this comment

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

Just because the frontend subtracts one and in backend (internally) we add one again. So, logic remains the same. And it's expected to pass all the tests.

Copy link
Copy Markdown
Collaborator Author

@Smit-create Smit-create Aug 29, 2022

Choose a reason for hiding this comment

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

Can you try applying this specific change to LFortran to make sure this works there as well.

Yep, tried. All tests passed for me. On the latest main branch of LFortran:

diff --git a/src/libasr/runtime/lfortran_intrinsics.c b/src/libasr/runtime/lfortran_intrinsics.c
index 17399c784..b71bfdd95 100644
--- a/src/libasr/runtime/lfortran_intrinsics.c
+++ b/src/libasr/runtime/lfortran_intrinsics.c
@@ -784,6 +784,7 @@ LFORTRAN_API char* _lfortran_str_copy(char* s, int32_t idx1, int32_t idx2) {
 LFORTRAN_API char* _lfortran_str_slice(char* s, int32_t idx1, int32_t idx2, int32_t step,
                         bool idx1_present, bool idx2_present) {
     int s_len = strlen(s);
+    idx2++;
     if (step == 0) {

Copy link
Copy Markdown
Collaborator

@czgdp1807 czgdp1807 Oct 5, 2022

Choose a reason for hiding this comment

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

Just make a PR there (in LFortran) as well with a similar test. We will merge both together.

if (step == 0) {
printf("slice step cannot be zero\n");
exit(1);
Expand Down
24 changes: 13 additions & 11 deletions src/lpython/semantics/python_ast_to_asr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2681,26 +2681,28 @@ class CommonVisitor : public AST::BaseVisitor<Struct> {
if (!ASRUtils::is_integer(*ASRUtils::expr_type(ASRUtils::EXPR(tmp)))) {
throw SemanticError("slice indices must be integers or None", tmp->loc);
}
ai.m_right = ASRUtils::EXPR(tmp);
// Subtract 1 from right since python has exclusive right limits.
ASR::ttype_t *a_type = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
4, nullptr, 0));
ASR::expr_t *constant_one = ASR::down_cast<ASR::expr_t>(ASR::make_IntegerConstant_t(
al, loc, 1, a_type));
ai.m_right = ASRUtils::EXPR(ASR::make_IntegerBinOp_t(al, loc, ASRUtils::EXPR(tmp),
ASR::binopType::Sub, constant_one, a_type, nullptr));
}
if (sl->m_step != nullptr) {
this->visit_expr(*sl->m_step);
if (!ASRUtils::is_integer(*ASRUtils::expr_type(ASRUtils::EXPR(tmp)))) {
throw SemanticError("slice indices must be integers or None", tmp->loc);
}
ai.m_step = ASRUtils::EXPR(tmp);
}
if( ai.m_left != nullptr &&
ASR::is_a<ASR::Var_t>(*ai.m_left) &&
ASR::is_a<ASR::Var_t>(*ai.m_right) ) {
ASR::Variable_t* startv = ASRUtils::EXPR2VAR(ai.m_left);
ASR::Variable_t* endv = ASRUtils::EXPR2VAR(ai.m_right);
is_item = is_item && (startv == endv);
} else {
is_item = is_item && (ai.m_left == nullptr &&
ai.m_step == nullptr &&
ai.m_right != nullptr);
ASR::ttype_t *a_type = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
4, nullptr, 0));
ASR::expr_t *constant_one = ASR::down_cast<ASR::expr_t>(ASR::make_IntegerConstant_t(
al, loc, 1, a_type));
ai.m_step = constant_one;
}
is_item = false;
if (ASR::is_a<ASR::List_t>(*type)) {
tmp = ASR::make_ListSection_t(al, loc, value, ai, type, nullptr);
return false;
Expand Down
2 changes: 1 addition & 1 deletion tests/reference/asr-list1-770ba33.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
"stderr": null,
"stderr_hash": null,
"returncode": 0
}
}