Fix ASR Slice frontend #1051

Smit-create · 2022-08-28T07:44:41Z

Fixes #332

Smit-create · 2022-08-28T07:47:57Z

The current PR works fine with this:

def main0():
    arr: i32[4]
    arr[0] = 1
    arr[1] = 0
    arr[2] = -4
    print(arr[:2])
    print(arr[1])
    print(arr[2])

main0()

Output:

The only issue is that the Array Section backend considers the right limit as inclusive whereas other Section backends (List/string) consider the right limit as exclusive. So we need to define some strict way of defining indexes, limits that is followed uniformly in all the backends.

czgdp1807 · 2022-08-28T15:24:44Z

The only issue is that the Array Section backend considers the right limit as inclusive whereas other

Well. Array is a shared feature between Fortran and Python. Fortran includes end index while slicing an array. However Python excludes end index. I think the backend should always treat the end index inclusive. Its the AST->ASR transition's job to format the indices of an array slices correctly. For example, array[0:3] should be converted to ArraySlice(array, 0, 2) in LPython's AST->ASR transition and ArraySlice(array, 0, 3) in LFortran's AST->ASR transition.

czgdp1807

Please add a test for your fix.

czgdp1807 · 2022-08-29T08:45:44Z

integration_tests/test_issue_332.py

+    print(arr[:2])
+    print(arr[1])
+    print(arr[2])


Use assert for verifying that the outputs are actually correct?

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

czgdp1807 · 2022-08-29T08:46:51Z

src/libasr/runtime/lfortran_intrinsics.c

 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++;


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.

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.

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) {

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

czgdp1807

Will shepherd this later.

Smit-create · 2022-09-19T15:44:25Z

Will shepherd this later.

@czgdp1807 Should we merge this? Can we add assertion tests as the follow-up PR to keep this clean?

certik · 2022-09-20T23:08:30Z

@czgdp1807 let me know what needs to get done here.

czgdp1807 · 2022-10-05T09:41:31Z

src/libasr/runtime/lfortran_intrinsics.c

 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++;


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

Smit-create · 2022-10-08T08:48:14Z

This is ready @czgdp1807

czgdp1807 · 2022-10-09T06:04:23Z

integration_tests/test_issue_332.py

+    print(arr[:2])
+    print(arr[1])
+    print(arr[2])


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

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

Makes sense. Can you try fixing the bug?

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

czgdp1807 · 2022-10-16T06:31:10Z

Please fix #1051 (comment) and resolve the conflicts. Will merge it then.

Smit-create requested review from czgdp1807 and certik August 28, 2022 07:48

czgdp1807 requested changes Aug 28, 2022

View reviewed changes

Smit-create force-pushed the i-332 branch from 0036833 to d3284f2 Compare August 29, 2022 06:19

czgdp1807 requested changes Aug 29, 2022

View reviewed changes

Smit-create requested a review from czgdp1807 August 29, 2022 10:53

czgdp1807 self-assigned this Aug 29, 2022

czgdp1807 reviewed Aug 29, 2022

View reviewed changes

Smit-create force-pushed the i-332 branch from d3284f2 to e870d52 Compare September 19, 2022 15:40

Smit-create requested a review from czgdp1807 September 19, 2022 15:44

Smit-create force-pushed the i-332 branch from e870d52 to 73bb1d3 Compare October 3, 2022 09:32

czgdp1807 requested changes Oct 5, 2022

View reviewed changes

czgdp1807 marked this pull request as draft October 5, 2022 09:41

Smit-create mentioned this pull request Oct 5, 2022

Fix lfortran_str_slice lfortran/lfortran#856

Merged

Smit-create marked this pull request as ready for review October 6, 2022 13:55

Smit-create requested a review from czgdp1807 October 6, 2022 13:55

Smit-create added the ready for review PRs that are ready for review label Oct 6, 2022

czgdp1807 requested changes Oct 9, 2022

View reviewed changes

czgdp1807 marked this pull request as draft October 16, 2022 06:30

czgdp1807 removed the ready for review PRs that are ready for review label Oct 16, 2022

Smit-create added 3 commits October 16, 2022 16:50

Do not consider slice object as item

3626b89

Make String Section backend inclusive for left and right limits.

3562fc0

Add and update tests

13f3b4b

Smit-create force-pushed the i-332 branch from 73bb1d3 to 13f3b4b Compare October 16, 2022 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ASR Slice frontend #1051

Fix ASR Slice frontend #1051

Smit-create commented Aug 28, 2022

Smit-create commented Aug 28, 2022

czgdp1807 commented Aug 28, 2022

czgdp1807 left a comment

czgdp1807 Aug 29, 2022

Smit-create Aug 29, 2022

czgdp1807 Aug 29, 2022

Smit-create Aug 29, 2022

Smit-create Aug 29, 2022

czgdp1807 Oct 5, 2022

czgdp1807 left a comment

Smit-create commented Sep 19, 2022

certik commented Sep 20, 2022

czgdp1807 Oct 5, 2022

Smit-create commented Oct 8, 2022

czgdp1807 Oct 9, 2022

Smit-create Oct 10, 2022

czgdp1807 Oct 14, 2022

Smit-create Oct 16, 2022

czgdp1807 commented Oct 16, 2022

Fix ASR Slice frontend #1051

Are you sure you want to change the base?

Fix ASR Slice frontend #1051

Conversation

Smit-create commented Aug 28, 2022

Smit-create commented Aug 28, 2022

czgdp1807 commented Aug 28, 2022

czgdp1807 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czgdp1807 left a comment

Choose a reason for hiding this comment

Smit-create commented Sep 19, 2022

certik commented Sep 20, 2022

Choose a reason for hiding this comment

Smit-create commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czgdp1807 commented Oct 16, 2022