Add list.reverse intrinsic function

[virendrakabra14]

Currently gives an Internal Compiler Error at

File ".../lpython/src/libasr/asr_utils.h", line 39
    return ASR::down_cast<ASR::stmt_t>(f);
AssertFailed: is_a<T>(*f)

with test

from lpython import i32

def test_list_reverse():
    x: list[i32]
    x = [1, 2]
    x.reverse()

test_list_reverse()

[certik]

I think this looks good.

[virendrakabra14]

I think the issue is that ASR.asdl has IntrinsicFunction only as expr, whereas list.reverse is a statement.

[czgdp1807]

I think the problem in the above comment and #1232 (comment) is exactly same. We might need to create Expr(expr expression) where expression will store IntrinsicFunction and then we can do make_Expr_t(...) to store this expression. @certik What do you say? I think this stores the complete information to go from ASR to LLVM and if needed back from ASR to Python code.

Another approach is that we can add IntrinsicFunctionStmt(int intrinsic_id, expr* args, int overload_id) in stmt in ASR.asdl. This would help in representing those intrinsic which don't return anything and are present just as a statement in the user code.

I guess the first approach is better because it helps in resolving #1232.

P.S. list.append, list.sort, list.clear will also face the same problem as is the case with list.reverse.

[certik]

I think adding a Expr(expr expression) statement node is the least disruptive approach for now and we can iterate on this later if this is not enough.

[virendrakabra14]

I added an Expr node, but errors remain the same. Looks like visit_Expr isn't called at all.

[czgdp1807]

Well you are not using it for wrapping the intrinsic function node in “create_ListReverse”.

[virendrakabra14]

Well you are not using it for wrapping the intrinsic function node in “create_ListReverse”.

I tried that

return ASR::make_Expr_t(al, loc, ASRUtils::EXPR(ASR::make_IntrinsicFunction_t(...)));

But we also need to allow similar to void return types for expressions.
Currently the issue is at visit_ttype.

[czgdp1807]

Yes. list.reverse won’t return anything. IntrinsicFuntionStmt seems tempting right now. Another way is to make presence of ttype type optional in IntrinsicFunction i.e., ttype type -> ttype? type and then in ListReverse::verify_args make sure that its nullptr. Another option is to introduce Void type in ASR. Best one seems to be Void but using ttype? type is also fine. @certik What do you say?

[certik]

I would use IntrinsicFunction.

    | IntrinsicFunction(int intrinsic_id, expr* args, int overload_id,
            ttype type, expr? value)                               

The type is FunctionType, which already allows the return type to be nil, so that is already supported. Regarding expr* args, @czgdp1807 why do we need it? It seems all the information we need is already represented in type, isn't it? Answer: we need it because IntrinsicFunction is not a declaration, but more like a IntrinsicFunctionCall. So those are the actual arguments.

Ok, so the current design already allows to "return nil". I don't think any change is needed? Maybe in ASR passes.

[czgdp1807]

Ok, so the current design already allows to "return nil". I don't think any change is needed?

There is one needed. The following one,

diff --git a/src/libasr/ASR.asdl b/src/libasr/ASR.asdl
index 12ada08a1..bdaa0ea1d 100644
--- a/src/libasr/ASR.asdl
+++ b/src/libasr/ASR.asdl
@@ -225,7 +225,7 @@ expr
     | FunctionCall(symbol name, symbol? original_name, call_arg* args,
             ttype type, expr? value, expr? dt)
     | IntrinsicFunction(int intrinsic_id, expr* args, int overload_id,
-            ttype type, expr? value)
+            ttype? type, expr? value)
     | StructTypeConstructor(symbol dt_sym, call_arg* args, ttype type, expr? value)
     | EnumTypeConstructor(symbol dt_sym, expr* args, ttype type, expr? value)
     | UnionTypeConstructor(symbol dt_sym, expr* args, ttype type, expr? value)

We do the same with return_var_type in FunctionType (see below),

lpython/src/libasr/ASR.asdl

Line 365 in bf59fb4

| FunctionType(ttype* arg_types, ttype? return_var_type,

I think after addition of Expr(expr expression) node we can try merging FunctionCall and SubroutineCall as well.

[certik]

I don't think that's the right change. The type is a FunctionType, so you always require this to be present, both for Functions (that return something) and Subroutines (that don't return anything). Inside the FunctionType the return_var_type is optional.

Oh, I just noticed that type is not FunctionType, but just the return value. I see.

I can see a couple issues here:

• I think the IntrinsicFunction should have a FunctionType somewhere, shouldn't it? It seems natural, but perhaps we don't need it?
• I am not 100% sure having type optional is good, since every expr requires a type. However, we can just enforce it in verify(), making sure every expr does have a type, and when IntrinsicFunction is used in stmt, then we check that type=nil.

So your change is probably ok after all. I was confused. I think I keep confusing that IntrinsicFunction is NOT like Function, but more like FunctionCall. We do not have an equivalent of Function for intrinsic functions, since it seems redundant, it seems the equivalent of FunctionCall is all we need, since the "Function" is intrinsic; to be precise: the equivalent of Function is the intrinsic_id and overload_id. I think that's a fine design for now. Perhaps later we can rename IntrinsicFunction to IntrinsicFunctionCall.

Here is one issue: what if we assign an intrinsic function to a function pointer? I don't know if Fortran allows it. Our current design does not allow it. We would have to "wrap" the intrinsic in a regular function. It's probably fine, since these intrinsic functions are often "generic", instantiated to the specific types. So we treat them as part of the language, not really "functions", but "intrinsic operations". The difference is that "intrinsic operations" are generic, instantiated, they do not have Function and can't be treated like functions. They do look like FunctionCall a bit, but without some of the other properties of functions.

[czgdp1807]

Everything looks good but please resolve the conflicts (update main locally and then merge into the branch of this PR, git will ask you to resolve the conflicts, do it, add and commit the changes). Do a clean build afterwards, see if all the tests pass and then push.

[czgdp1807]

Remove this line.

[czgdp1807]

Seems like some syntax creeped in while resolving conflicts. @kabra1110 Do you need any help in fixing or would you able to deal with it on your own.

[czgdp1807]

@certik Does this look good to you?

[certik]

I think that's fine.