ENH: support runtime dim for intrinsic sum

[Pranavchiku]

Fixes #3540

[Pranavchiku]

this will be same, I do not think there can be any other variations possible for this function.

[certik]

What does this function do?

[Pranavchiku]

Ensures return type do not have variables from other scope. As it is passed from create_Sum to instantiate_Sum, it has parameters from different scope.

[certik]

Can you document this as a comment?

[Pranavchiku]

We will have to map corresponding arguments based on intrinsic function we are replacing.

[certik]

This is unrelated: I wonder if merge can be just an elemental function.

[czgdp1807]

merge is an elemental function.

[certik]

Is this deallocated properly?

[Pranavchiku]

Yes, it gets implicitly deallocated in pass insert_deallocate

deallocate(__libasr__created__var__0_Sum_4_2_1_res) ! Implicit deallocate

[czgdp1807]

Actually this is problematic in general (not here only but other places as well). We need to allocate only when extremely necessary like when there is no other way left. Same for creating auxiliary variables, create only when extremely necessary. Otherwise they lead to a lot of bugs.

[certik]

How is this runtime dim feature implemented? I see just a slight modification there to use merge for the return argument, which is good. But how is the loop structure modified? My understanding is that currently we decide at compile time what the loop structure will be, but now we need to decide at runtime, but I don't see any such runtime implementation.

We also need to improve tests, as indicated above. Those more robust tests would reveal any possible issues with the implementation.

Overall great job, thanks for tackling this difficult feature! Let's iron out these details.

[Pranavchiku]

But how is the loop structure modified?

Implementation of sum is independent of compile time value of dim, so we need not to modify anything.

lfortran/src/libasr/asr_builder.h

Lines 723 to 739 in 755ad60

	void generate_reduction_intrinsic_stmts_for_array_output(const Location& loc,
	ASR::expr_t* array, ASR::expr_t* dim, SymbolTable* fn_scope,
	Vec<ASR::stmt_t*>& fn_body, Vec<ASR::expr_t*>& idx_vars,
	Vec<ASR::expr_t*>& target_idx_vars, Vec<ASR::stmt_t*>& doloop_body,
	INIT init_stmts, LOOP_BODY loop_body) {
	init_stmts();
	int n_dims = ASRUtils::extract_n_dims_from_ttype(ASRUtils::expr_type(array));
	ASR::stmt_t** else_ = nullptr;
	size_t else_n = 0;
	idx_vars.reserve(al, n_dims);
	PassUtils::create_idx_vars(idx_vars, n_dims, loc, al, fn_scope, "_j");
	for( int i = 1; i <= n_dims; i++ ) {
	ASR::expr_t* current_dim = i32(i);
	ASR::expr_t* test_expr = make_Compare(make_IntegerCompare_t, dim,
	Eq, current_dim);
	Vec<ASR::expr_t*> loop_vars;

we decide at compile time what the loop structure will be

For intrinsic Count, we have this problem, but it is problem in implementation which needs to resolved separately.

[certik]

I think that this is good enough to merge. If there are bugs, we'll implement them.

Thanks for implementing this!

[czgdp1807]

This is fine in general.