[ASR/LLVM]: Implement Tuple/List Compare

[Smit-create]

#1245

[Smit-create]

This PR adds to the ASR front end. We need to figure out how to handle it in the LLVM backend. C handles this recursively and I suppose that would work fine with the current C implementations.

[czgdp1807]

Please implement the support for both these nodes in LLVM. I will approve and merge then.

[Smit-create]

Yes, I'm planning to add a pass to handle the tuple/list comparison.

[Smit-create]

This PR needs #1349 to be merged

[czgdp1807]

Test for nested tuples as well with mixed types like, tuple[i32, tuple[i64, f32], str].

[czgdp1807]

This case (see #1342 (comment)) is not yet covered in the tests. Please test for this first.

[Smit-create]

This case is not yet covered in the tests. Please test for this first.

I avoided using f32 because of precision issues in comparison. I added a test for the mixed types:

    a: tuple[tuple[i32, i32], str, bool]
    b: tuple[tuple[i32, i32], str, bool]
    a = (a11, 'ok', True)
    b = (b11, 'ok', True)
    assert a == b
    b = (b11, 'notok', True)
    assert a != b

[certik]

Can you put an example here what is an example input and example output of this pass?

[Smit-create]

Thanks, I will document once I complete adding the ListCompare pass.

[certik]

This adds a pass in the LLVM backend. What exactly does the pass do? Please document in the docstring, see my comment above.

Is this just an optional pass that we currently run to simplify the backend, but it is not needed to run in theory? Or is this pass required to always be run due to a deficiency in our ASR design? If the latter, then we need to improve ASR, so that one can generate code from it without requiring any passes.

[czgdp1807]

If the latter, then we need to improve ASR, so that one can generate code from it without requiring any passes.

IMHO, we don't need a pass here at all (at least for tuples). The backend should be able to do this. Comparing tuples is fairly simple in LLVM. Just generate code according to the type of the elements (they are fixed in number as tuples are fixed size types, we specify everything in the tuple type itself). Calling function for simple comparison of tuples is not worth it I think. Comparing lists though does require generating a loop but I think for generating low level code like LLVM, x86 the loop should be inlined. For C we can add function on our own. Pass might not be needed.

[Smit-create]

The reason for adding a pass to compare complex data types like lists, tuples is that it will make the LLVM backend less complex. The current approach recursively compares each element for a tuple, list, etc, and altogether is implemented under one class. The other benefit is that if we make this pass re-usable to any other backend will reduce the effort to write the same recursive algorithm in each backend.

IMHO, we don't need a pass here at all (at least for tuples)

We need TupleCompare and ListCompare at the same level (either both at ASR pass or both at the backend.) Otherwise, the nested datatypes won't work effectively. For example, if we implement ListCompare via a pass and TupleCompare in LLVM backend then, tuple[list[str], i32] won't work.

[czgdp1807]

The reason for adding a pass to compare complex data types like lists, tuples is that it will make the LLVM backend less complex

The problem is LLVM already has the infrastructure to generate code for Tuple comparisons. Extending it for List comparisons is easy. We need it to implement dict effectively. So having or not having this pass doesn't make much difference. You may use this pass for high level languages like C, C++, Julia, Fortran backends. But for low level backends like LLVM, x86 I would vouch for directly generating inlined code.

Let's do the following,

1. For LLVM backend, open a separate PR and implement these comparisons directly there. Reason being simple, we already compare tuples in LLVM for implementing dict, so just re-using those APIs would be easy. See below,

lpython/src/libasr/codegen/llvm_utils.cpp

Lines 234 to 235 in 81cf7de

	llvm::Value* LLVMUtils::is_equal_by_value(llvm::Value* left, llvm::Value* right,
	llvm::Module& module, ASR::ttype_t* asr_type) {

Extend it. Re-use it. Modify it. It will generate inlined code so no overhead of function calls.

2. For C, C++ keep using this pass as it will require less effort there and generate readable code for high level languages backends.

[Smit-create]

Sure, that sounds like a good idea. I can add a pass for both the compares in this PR and open a new PR for the LLVM backend as a follow-up.

[Smit-create]

@certik @czgdp1807 I have added a pass for both list and tuple compare. We can merge this if this is okay and then as a follow-up, I'll try to remove this pass from the default list and then implement in the LLVM backend.

[czgdp1807]

I'll try to remove this pass from the default list and then implement in the LLVM backend.

I don't like the pass for LLVM backend but I will approve this PR. Please add it immediately after this PR is merged.

[czgdp1807]

This case (see #1342 (comment)) is not yet covered in the tests. Please test for this first.

[Smit-create]

Ahh, this was working all good before rebasing but now fails with this:

this->visit_stmt(*x.m_body[i]);
  File "../libasr/asr.h", line 4217
  File "../libasr/asr.h", line 3983
  File "/Users/thebigbool/repos/lpython/src/libasr/codegen/asr_to_llvm.cpp", line 4007
    LFORTRAN_ASSERT(finder != nested_globals.end());
AssertFailed: finder != nested_globals.end()