Replace BlockCall symbol with the one in current scope

[Thirumalai-Shaktivel]

Fixes: #1668

[Thirumalai-Shaktivel]

The issue was in the inline_function_calls pass:

deffunc() ->i32:
return30deftest() ->i32:
temp: i32=func()
returntempx : i32x=test()
print(x)

(TranslationUnit
(SymbolTable1
{
_global_symbols:
(Module
(SymbolTable6
{
_lpython_main_program:
(Function
(SymbolTable5
{
_lpython_return_variable_func_test:
(Variable5
_lpython_return_variable_func_test
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
_lpython_return_variable_test:
(Variable5
_lpython_return_variable_test
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
temp_test:
(Variable5
temp_test
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
~empty_block:
(Block
(SymbolTable8
{
})
~empty_block
[]
)
})
_lpython_main_program
(FunctionType
[]
()
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
)
[]
[]
[(=
(Var5 _lpython_return_variable_func_test)
(IntegerConstant30 (Integer4 []))
()
)
(GoTo1
__1
)
(BlockCall13 ~empty_block ! < ------------- Here is the issue
)
(=
(Var5 temp_test)
(Var5 _lpython_return_variable_func_test)
()
)
(=
(Var5 _lpython_return_variable_test)
(Var5 temp_test)
()
)
(GoTo2
__2
)
(BlockCall25 ~empty_block
)
(=
(Var6 x)
(Var5 _lpython_return_variable_test)
()
)
(Print
()
[(Var6 x)]
()
()
)]
()
Public
.false.
.false.
),
func:
(Function
(SymbolTable2
{
_lpython_return_variable:
(Variable2
_lpython_return_variable
[]
ReturnVar
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
)
})
func
(FunctionType
[]
(Integer4 [])
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
)
[]
[]
[(=
(Var2 _lpython_return_variable)
(IntegerConstant30 (Integer4 []))
()
)
(Return)]
(Var2 _lpython_return_variable)
Public
.false.
.false.
),
test:
(Function
(SymbolTable3
{
_lpython_return_variable:
(Variable3
_lpython_return_variable
[]
ReturnVar
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
_lpython_return_variable_func:
(Variable3
_lpython_return_variable_func
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
temp:
(Variable3
temp
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
),
~empty_block:
(Block
(SymbolTable7
{
})
~empty_block
[]
)
})
test
(FunctionType
[]
(Integer4 [])
Source
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
)
[]
[]
[(=
(Var3 _lpython_return_variable_func)
(IntegerConstant30 (Integer4 []))
()
)
(GoTo1
__1
)
(BlockCall13 ~empty_block
)
(=
(Var3 temp)
(Var3 _lpython_return_variable_func)
()
)
(=
(Var3 _lpython_return_variable)
(Var3 temp)
()
)
(Return)]
(Var3 _lpython_return_variable)
Public
.false.
.false.
),
x:
(Variable6
x
[]
Local
()
()
Default
(Integer4 [])
Source
Public
Required
.false.
)
})
_global_symbols
[]
.false.
.false.
),
main_program:
(Program
(SymbolTable4
{
_lpython_main_program:
(ExternalSymbol4
_lpython_main_program
6 _lpython_main_program
_global_symbols
[]
_lpython_main_program
Public
)
})
main_program
[_global_symbols]
[(SubroutineCall4 _lpython_main_program
()
[]
()
)]
)
})
[]
)

Here the statements from the func function were copied into the test() function (which included the BlockCall and Goto statements used by the previous function inline.)

[gptsarthak]

Thank you. Please check if this also fixes #1677

[certik]

We need to be careful here, I think you want to duplicate it, don't you?

[Thirumalai-Shaktivel]

I thought the BlockCall and Goto is specific to one function.

If we duplicate it, should we create an ExternalSymbol for the Block?

...
_lpython_main_program:
(Function
(SymbolTable5
{
...
(BlockCall13 ~empty_block ! < ---- This is created in another function symtab
)
...
(BlockCall25 ~empty_block
)
...

[certik]

I don't know. I agree that goto is specific to a function. But removing it seems to make the code incorrect. So we need to figure out how to solve this.

[czgdp1807]

Correct. We need to duplicate it and then edit BlockCall and Goto statements in the duplicated body. It should be doable and not that hard AFAICT as of now.

[Thirumalai-Shaktivel]

I'm not sure if this new change was a correct fix.

What does the Block symbol store?
What is BlockCall used for?

[czgdp1807]

Instead of doing the edit here, you need to follow the same pattern as in visit_Var (add visit_BlockCall and then edit its symbol, if the code is exactly the same then add a macro to generate code for each method).

lpython/src/libasr/pass/inline_function_calls.cpp

Lines 98 to 123 in 1f60cf0

	voidvisit_Var(constASR::Var_t& x) {
	ASR::Var_t& xx = const_cast<ASR::Var_t&>(x);
	std::string x_var_name = std::string(ASRUtils::symbol_name(x.m_v));
	// If anything is not local to a function being inlined
	// then do not inline the function by setting
	// fixed_duplicated_expr_stmt to false.
	// To be supported later.
	if( current_routine_scope &&
	current_routine_scope->get_symbol(x_var_name) == nullptr ) {
	fixed_duplicated_expr_stmt = false;
	return ;
	}
	if( x.m_v->type == ASR::symbolType::Variable ) {
	ASR::Variable_t* x_var = ASR::down_cast<ASR::Variable_t>(x.m_v);
	if( arg2value.find(x_var_name) != arg2value.end() ) {
	x_var = ASR::down_cast<ASR::Variable_t>(arg2value[x_var_name]);
	if( current_scope->get_symbol(std::string(x_var->m_name)) != nullptr ) {
	xx.m_v = arg2value[x_var_name];
	}
	x_var = ASR::down_cast<ASR::Variable_t>(x.m_v);
	}
	} else {
	fixed_duplicated_expr_stmt = false;
	}
	}

[certik]

I think the bug is in the inline pass. It must create a unique name for the ~empty_block block.

[certik]

Rather, it looks like the issue is that the Block symbol points to the old location and BlockCall is calling it from the old location, so one gets the following error message:

ASR verify pass error: ASR verify: Block ~empty_block should resolve in current scope.

caused by:

 void visit_BlockCall(const BlockCall_t& x) {
require(x.m_m != nullptr, "Block call made to inexisting block");
require(symtab_in_scope(current_symtab, x.m_m),
"Block " + std::string(ASRUtils::symbol_name(x.m_m)) +
" should resolve in current scope.");
...

So it looks like the nested function pass needs to copy over the Block symbol (duplicate).

[czgdp1807]

#1688 (comment) is the correct approach. If one is not doing it fully then ASR verify pass errors will always be there. Also I think editing is not happening correctly for BlockCall. https://github.com/lcompilers/lpython/pull/1688/files#r1161810225 isn’t addressed yet.

[Thirumalai-Shaktivel]

This seems to fix the issue.
@czgdp1807 Please review this carefully; I removed some assignments that were not used. If you feel like reverting something, let me know, and I will make the required changes.

[czgdp1807]

Please review this carefully; I removed some assignments that were not used. If you feel like reverting something, let me know, and I will make the required changes.

If you are not sure then apply changes to LFortran and see if they work there. If they do then we are good. More testing should ensure that we are not facing any bugs.

[czgdp1807]

Also, I think we should have a llvm-fast option in integration_tests/CMakeLists.txt. We should enable it in all the tests possible to make sure that our optimisation passes are also working as expected all the times.

[certik]

Yes, we need --fast testing in both LFortran and LPython for all llvm tests. I created an issue for this at lfortran/lfortran#1560.

[Thirumalai-Shaktivel]

LFortran PR: lfortran/lfortran#1566