Add provision to visit_FunctionCall in update_call_args

[Pranavchiku]

With this we get correct ASR for #1977 (comment), however the error still persists. On debugging, I found that llvm_symtab_fn_arg consists hash corresponding to f present in symbol table of a, and then when we encounter call a(f) in body of b, f is part of symbol table belonging to b and thus it throws error.

One way to fix this is, whenever we add hash of an external function into llvm_symtab_fn_arg, lookup all the symtabs where it is present and add their hash as well. Although, we do not have any information to check if is_external(f).

Another way that comes to my mind is to make all external functions part of module __lcompilers_external_functions_module, this way we have a global module accessible to every subroutine but, this will have a large blast radius ( handling ExternalSymbol, the same we faced in #2396 ).

subroutine b()
   external f
   call a(f)
end subroutine

subroutine a(f)
   real r
   external f
   r = f(2.0)
   print *, r
end subroutine

program main
   call b()
end program

LFortran

% lfortran ./tests/external_03.f90 --implicit-typing --implicit-interface 
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 2307
    err = compile_to_object_file(arg_file, tmp_o, false,
  File "/Users/pranavchiku/repos/lfortran/src/bin/lfortran.cpp", line 930
    res = fe.get_llvm3(*asr, lpm, lm, diagnostics, infile);
  File "/Users/pranavchiku/repos/lfortran/src/lfortran/fortran_evaluator.cpp", line 358
    = asr_to_llvm(asr, diagnostics,
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 9220
    v.visit_asr((ASR::asr_t&)asr);
  File "../libasr/asr.h", line 5057
  File "../libasr/asr.h", line 5033
  File "../libasr/asr.h", line 5058
  File "../libasr/asr.h", line 4766
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 938
    visit_symbol(*item.second);
  File "../libasr/asr.h", line 5060
  File "../libasr/asr.h", line 4775
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 3673
    visit_procedures(x);
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 3870
    this->visit_stmt(*x.m_body[i]);
  File "../libasr/asr.h", line 5077
  File "../libasr/asr.h", line 4825
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 8388
    std::vector<llvm::Value *> args2 = convert_call_args(x, is_method);
  File "/Users/pranavchiku/repos/lfortran/src/libasr/codegen/asr_to_llvm.cpp", line 7870
    LCOMPILERS_ASSERT(llvm_symtab_fn_arg.find(h) != llvm_symtab_fn_arg.end());
AssertFailed: llvm_symtab_fn_arg.find(h) != llvm_symtab_fn_arg.end()

[certik]

It seems the ASR is incorrect if f is living in two different symbol tables. I think it must be exactly the same f interface living in the global scope.

I will review this PR carefully later today.

[certik]

I think the code is fine otherwise. I checked the ASR for the new test you added and it seems there are two definitions of the function "f". I see two ways forward:

• These two interfaces for f must be exactly the same. We need to check if that works with our LLVM backend, since the declaration will be duplicated, but if it is exactly the same, that might be ok.
• We need to put this interface f into one place, either global scope, or some automatically created module.

[Pranavchiku]

It seems the ASR is incorrect if f is living in two different symbol tables

I checked the ASR for the new test you added and it seems there are two definitions of the function "f"

This is expected, no? The way we tackle external function is we have separate symbols defined in each symtab as Interface and BindC and then LLVM appends .* to identify those uniquely. Pass unique_call_args make sure that every external function has same definition if it is updated somehow and this PR adds provision to update external function when change is done via FunctionCall.

[certik]

Are the two declarations of f exactly the same?

[Pranavchiku]

Are the two declarations of f exactly the same?

Yes.

[certik]

Ok, so let's merge this PR after you fix the macro issue.

Then the next step is to improve the LLVM backend to generate correct code. If f is the same, then either one can be used to create the LLVM interface, and then the exact same interface must be used in the second function --- that should work (assuming the interface is exactly the same, which it is now). If it doesn't, let's investigate.

[Pranavchiku]

If f is the same, then either one can be used to create the LLVM interface, and then the exact same interface must be used in the second function

The only trouble is both f being in different symtabs implying different hash, and hence it leads to assertion error llvm_symtab_fn_arg.find(h) != llvm_symtab_fn_arg.end(). We might need to think how can we tie all external functions to same hash or maybe populate llvm_symtab_fn_arg for other fs when a f is encountered.

[certik]

I think the PR is fine.

[certik]

The only trouble is both f being in different symtabs implying different hash, and hence it leads to assertion error llvm_symtab_fn_arg.find(h) != llvm_symtab_fn_arg.end(). We might need to think how can we tie all external functions to same hash or maybe populate llvm_symtab_fn_arg for other fs when a f is encountered.

Why cannot the two "f" be treated like two different functions with two different hashes? They should generate the same signature.

[Pranavchiku]

Why cannot the two "f" be treated like two different functions with two different hashes?

Yes, they can be treated. In this case, the only place where llvm_symtab_fn_arg gets populated is declare_args which gets called only via define_function_entry called via generate_function through visit_Function. So now, if you look at subroutine b, there is now way we can have hash(f) in llvm_symtab_fn_arg.

subroutine b()
   external f
   call a(f)
end subroutine

subroutine a(f)
   real r
   external f
   r = f(2.0)
   print *, r
end subroutine

So we need to somehow bypass the assertion for this kind of special cases.

[Pranavchiku]

with the following diff, we have it working

diff --git a/src/libasr/codegen/asr_to_llvm.cpp b/src/libasr/codegen/asr_to_llvm.cpp
index 4617fd5b5..bf8e45434 100644
--- a/src/libasr/codegen/asr_to_llvm.cpp
+++ b/src/libasr/codegen/asr_to_llvm.cpp
@@ -7896,6 +7896,12 @@ public:
                     if (ASRUtils::get_FunctionType(fn)->m_deftype == ASR::deftypeType::Implementation) {
                         LCOMPILERS_ASSERT(llvm_symtab_fn.find(h) != llvm_symtab_fn.end());
                         tmp = llvm_symtab_fn[h];
+                    } else if (llvm_symtab_fn_arg.find(h) == llvm_symtab_fn_arg.end() &&
+                                ASR::is_a<ASR::Function_t>(*var_sym) &&
+                                ASRUtils::get_FunctionType(fn)->m_deftype == ASR::deftypeType::Interface ) {
+                        LCOMPILERS_ASSERT(llvm_symtab_fn.find(h) != llvm_symtab_fn.end());
+                        tmp = llvm_symtab_fn[h];
+                        // LCOMPILERS_ASSERT(tmp != nullptr)
                     } else {
                         // Must be an argument/chained procedure pass
                         LCOMPILERS_ASSERT(llvm_symtab_fn_arg.find(h) != llvm_symtab_fn_arg.end());

% lfortran c.f90 --implicit-interface --show-llvm
; ModuleID = 'LFortran'
source_filename = "LFortran"

@0 = private unnamed_addr constant [2 x i8] c" \00", align 1
@1 = private unnamed_addr constant [2 x i8] c"\0A\00", align 1
@2 = private unnamed_addr constant [9 x i8] c"%13.8e%s\00", align 1

define void @a(float (float*)* %f) {
.entry:
  %call_arg_value = alloca float, align 4
  %r = alloca float, align 4
  store float 2.000000e+00, float* %call_arg_value, align 4
  %0 = call float %f(float* %call_arg_value)
  store float %0, float* %r, align 4
  %1 = load float, float* %r, align 4
  %2 = fpext float %1 to double
  call void (i8*, ...) @_lfortran_printf(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @2, i32 0, i32 0), double %2, i8* getelementptr inbounds ([2 x i8], [2 x i8]* @1, i32 0, i32 0))
  br label %return

return:                                           ; preds = %.entry
  ret void
}

declare float @f(float*)

define void @b() {
.entry:
  call void @a(float (float*)* @f.1)
  br label %return

return:                                           ; preds = %.entry
  ret void
}

declare float @f.1(float*)

declare void @_lfortran_printf(i8*, ...)

define i32 @main(i32 %0, i8** %1) {
.entry:
  call void @_lpython_set_argv(i32 %0, i8** %1)
  call void @b()
  ret i32 0
}

declare void @_lpython_set_argv(i32, i8**)

[certik]

Ok, submit a PR.