Skip to content

Enable C test for test_math.py #1357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Enable C test for test_math.py #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9ef293
wip
czgdp1807 Dec 12, 2022
a8ab745
done
czgdp1807 Dec 16, 2022
90a8fe1
Use overload to avoid incorrect linking of symbols
czgdp1807 Dec 16, 2022
45b1b3b
done
czgdp1807 Dec 16, 2022
f1b31d8
Use constant variables to declare import constants inside a Function
czgdp1807 Jan 10, 2023
d5517c2
done
czgdp1807 Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions integration_tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,8 @@ RUN(NAME test_import_01 LABELS cpython llvm c)
RUN(NAME test_import_02 LABELS cpython llvm c)
RUN(NAME test_import_03 LABELS cpython llvm c)
RUN(NAME test_import_04 IMPORT_PATH ..
LABELS cpython llvm c)
RUN(NAME test_math LABELS cpython llvm)
LABELS cpython llvm)
RUN(NAME test_math LABELS cpython llvm c)
RUN(NAME test_numpy_01 LABELS cpython llvm c)
RUN(NAME test_numpy_02 LABELS cpython llvm c)
RUN(NAME test_numpy_03 LABELS cpython llvm c)
Expand Down
66 changes: 56 additions & 10 deletions src/libasr/codegen/asr_to_c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,13 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
counter += 1;
ASR::dimension_t* m_dims = nullptr;
size_t n_dims = ASRUtils::extract_dimensions_from_ttype(mem_type, m_dims);
sub += indent + convert_variable_decl(*mem_var, true, true, true, true, mem_var_name) + ";\n";
CDeclarationOptions c_decl_options_;
c_decl_options_.pre_initialise_derived_type = true;
c_decl_options_.use_ptr_for_derived_type = true;
c_decl_options_.use_static = true;
c_decl_options_.force_declare = true;
c_decl_options_.force_declare_name = mem_var_name;
sub += indent + convert_variable_decl(*mem_var, &c_decl_options_) + ";\n";
if( !ASRUtils::is_fixed_size_array(m_dims, n_dims) ) {
sub += indent + name + "->" + itr.first + " = " + mem_var_name + ";\n";
}
Expand All @@ -172,12 +178,34 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
}

std::string convert_variable_decl(const ASR::Variable_t &v,
bool pre_initialise_derived_type=true,
bool use_ptr_for_derived_type=true,
bool use_static=true,
bool force_declare=false,
std::string force_declare_name="")
DeclarationOptions* decl_options=nullptr)
{
bool pre_initialise_derived_type;
bool use_ptr_for_derived_type;
bool use_static;
bool force_declare;
std::string force_declare_name;
bool declare_as_constant;
std::string const_name;

if( decl_options ) {
CDeclarationOptions* c_decl_options = reinterpret_cast<CDeclarationOptions*>(decl_options);
pre_initialise_derived_type = c_decl_options->pre_initialise_derived_type;
use_ptr_for_derived_type = c_decl_options->use_ptr_for_derived_type;
use_static = c_decl_options->use_static;
force_declare = c_decl_options->force_declare;
force_declare_name = c_decl_options->force_declare_name;
declare_as_constant = c_decl_options->declare_as_constant;
const_name = c_decl_options->const_name;
} else {
pre_initialise_derived_type = true;
use_ptr_for_derived_type = true;
use_static = true;
force_declare = false;
force_declare_name = "";
declare_as_constant = false;
const_name = "";
}
std::string sub;
bool use_ref = (v.m_intent == LFortran::ASRUtils::intent_out ||
v.m_intent == LFortran::ASRUtils::intent_inout);
Expand Down Expand Up @@ -275,8 +303,13 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
}
} else {
bool is_fixed_size = true;
std::string v_m_name = v.m_name;
if( declare_as_constant ) {
type_name = "const " + type_name;
v_m_name = const_name;
}
dims = convert_dims_c(t->n_dims, t->m_dims, v_m_type, is_fixed_size);
sub = format_type_c(dims, type_name, v.m_name, use_ref, dummy);
sub = format_type_c(dims, type_name, v_m_name, use_ref, dummy);
}
} else if (ASRUtils::is_real(*v_m_type)) {
ASR::Real_t *t = ASR::down_cast<ASR::Real_t>(v_m_type);
Expand Down Expand Up @@ -307,8 +340,13 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
}
} else {
bool is_fixed_size = true;
std::string v_m_name = v.m_name;
if( declare_as_constant ) {
type_name = "const " + type_name;
v_m_name = const_name;
}
dims = convert_dims_c(t->n_dims, t->m_dims, v_m_type, is_fixed_size);
sub = format_type_c(dims, type_name, v.m_name, use_ref, dummy);
sub = format_type_c(dims, type_name, v_m_name, use_ref, dummy);
}
} else if (ASRUtils::is_complex(*v_m_type)) {
headers.insert("complex");
Expand Down Expand Up @@ -340,8 +378,13 @@ class ASRToCVisitor : public BaseCCPPVisitor<ASRToCVisitor>
}
} else {
bool is_fixed_size = true;
std::string v_m_name = v.m_name;
if( declare_as_constant ) {
type_name = "const " + type_name;
v_m_name = const_name;
}
dims = convert_dims_c(t->n_dims, t->m_dims, v_m_type, is_fixed_size);
sub = format_type_c(dims, type_name, v.m_name, use_ref, dummy);
sub = format_type_c(dims, type_name, v_m_name, use_ref, dummy);
}
} else if (ASRUtils::is_logical(*v_m_type)) {
ASR::Logical_t *t = ASR::down_cast<ASR::Logical_t>(v_m_type);
Expand Down Expand Up @@ -731,12 +774,15 @@ R"(
indentation_level += 1;
std::string open_struct = indent + c_type_name + " " + std::string(x.m_name) + " {\n";
indent.push_back(' ');
CDeclarationOptions c_decl_options_;
c_decl_options_.pre_initialise_derived_type = false;
c_decl_options_.use_ptr_for_derived_type = false;
for( size_t i = 0; i < x.n_members; i++ ) {
ASR::symbol_t* member = x.m_symtab->get_symbol(x.m_members[i]);
LFORTRAN_ASSERT(ASR::is_a<ASR::Variable_t>(*member));
body += indent + convert_variable_decl(
*ASR::down_cast<ASR::Variable_t>(member),
false, false);
&c_decl_options_);
if( !ASR::is_a<ASR::Const_t>(*ASRUtils::symbol_type(member)) ||
ASR::down_cast<ASR::Variable_t>(member)->m_intent == ASRUtils::intent_return_var ) {
body += ";\n";
Expand Down
69 changes: 64 additions & 5 deletions src/libasr/codegen/asr_to_c_cpp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@

namespace LFortran {


// Platform dependent fast unique hash:
static inline uint64_t get_hash(ASR::asr_t *node)
{
Expand All @@ -43,6 +42,38 @@ struct SymbolInfo
bool intrinsic_function = false;
};

struct DeclarationOptions {
};

struct CDeclarationOptions: public DeclarationOptions {
bool pre_initialise_derived_type;
bool use_ptr_for_derived_type;
bool use_static;
bool force_declare;
std::string force_declare_name;
bool declare_as_constant;
std::string const_name;

CDeclarationOptions() :
pre_initialise_derived_type{true},
use_ptr_for_derived_type{true},
use_static{true},
force_declare{false},
force_declare_name{""},
declare_as_constant{false},
const_name{""} {
}
};

struct CPPDeclarationOptions: public DeclarationOptions {
bool use_static;
bool use_templates_for_arrays;

CPPDeclarationOptions() :
use_static{true},
use_templates_for_arrays{false} {
}
};

template <class Struct>
class BaseCCPPVisitor : public ASR::BaseVisitor<Struct>
Expand Down Expand Up @@ -376,9 +407,14 @@ R"(#include <stdio.h>
ASR::Variable_t *arg = LFortran::ASRUtils::EXPR2VAR(x.m_args[i]);
LFORTRAN_ASSERT(LFortran::ASRUtils::is_arg_dummy(arg->m_intent));
if( is_c ) {
func += self().convert_variable_decl(*arg, false);
CDeclarationOptions c_decl_options;
c_decl_options.pre_initialise_derived_type = false;
func += self().convert_variable_decl(*arg, &c_decl_options);
} else {
func += self().convert_variable_decl(*arg, false, true);
CPPDeclarationOptions cpp_decl_options;
cpp_decl_options.use_static = false;
cpp_decl_options.use_templates_for_arrays = true;
func += self().convert_variable_decl(*arg, &cpp_decl_options);
}
if (i < x.n_args-1) func += ", ";
}
Expand Down Expand Up @@ -458,6 +494,29 @@ R"(#include <stdio.h>
}
}

for (auto &item : x.m_symtab->get_scope()) {
ASR::symbol_t* var_sym = item.second;
if( ASR::is_a<ASR::ExternalSymbol_t>(*var_sym) ) {
ASR::ExternalSymbol_t* v_ext = ASR::down_cast<ASR::ExternalSymbol_t>(var_sym);
ASR::symbol_t* v_sym = v_ext->m_external;
if (ASR::is_a<ASR::Variable_t>(*v_sym)) {
ASR::Variable_t* v = ASR::down_cast<ASR::Variable_t>(v_sym);
if( v->m_symbolic_value ) {
if( is_c ) {
CDeclarationOptions c_decl_options;
c_decl_options.declare_as_constant = true;
c_decl_options.const_name = v_ext->m_name;
decl += indent + self().convert_variable_decl(*v, &c_decl_options) + ";\n";
} else {
// TODO: Do for CPP when the use case shows up
decl += indent + self().convert_variable_decl(*v) + ";\n";
}
src.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to be careful here.

What happens if v_ext->m_name is equal to "x" and then we have another variable like "expr, will the x` in it be substituted? I think it will be, so we do not want to do that. Also we do not want to have any such macro defines in header files. We currently do not generate them, but eventually we will.

I would not use C macros.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not use C macros.

So constant C variables should work for this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that, or we can initialize the global variables when the program starts. I think both seems better than macros.

}
}
}
}

current_function = &x;

for (size_t i=0; i<x.n_body; i++) {
Expand Down Expand Up @@ -1099,9 +1158,9 @@ R"(#include <stdio.h>
sv->m_intent == ASRUtils::intent_inout) &&
is_c && ASRUtils::is_array(sv->m_type) &&
ASRUtils::is_pointer(sv->m_type)) {
src = "(*" + std::string(ASR::down_cast<ASR::Variable_t>(s)->m_name) + ")";
src = "(*" + std::string(ASRUtils::symbol_name(x.m_v)) + ")";
} else {
src = std::string(ASR::down_cast<ASR::Variable_t>(s)->m_name);
src = std::string(ASRUtils::symbol_name(x.m_v));
}
last_expr_precedence = 2;
}
Expand Down
15 changes: 13 additions & 2 deletions src/libasr/codegen/asr_to_cpp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,20 @@ class ASRToCPPVisitor : public BaseCCPPVisitor<ASRToCPPVisitor>
return typename_T + "* " + v_name;
}

std::string convert_variable_decl(const ASR::Variable_t &v, bool use_static=true,
bool use_templates_for_arrays=false)
std::string convert_variable_decl(const ASR::Variable_t &v, DeclarationOptions* decl_options=nullptr)
{
bool use_static;
bool use_templates_for_arrays;

if( decl_options ) {
CPPDeclarationOptions* cpp_decl_options = reinterpret_cast<CPPDeclarationOptions*>(decl_options);
use_static = cpp_decl_options->use_static;
use_templates_for_arrays = cpp_decl_options->use_templates_for_arrays;
} else {
use_static = true;
use_templates_for_arrays = false;
}

std::string sub;
bool use_ref = (v.m_intent == LFortran::ASRUtils::intent_out ||
v.m_intent == LFortran::ASRUtils::intent_inout ||
Expand Down
10 changes: 10 additions & 0 deletions src/libasr/runtime/lfortran_intrinsics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,16 @@ LFORTRAN_API double _lfortran_dlog(double x)
return log(x);
}

LFORTRAN_API float _lfortran_slog1p(float x)
{
return log1pf(x);
}

LFORTRAN_API double _lfortran_dlog1p(double x)
{
return log1p(x);
}

LFORTRAN_API float_complex_t _lfortran_clog(float_complex_t x)
{
return clogf(x);
Expand Down
2 changes: 2 additions & 0 deletions src/libasr/runtime/lfortran_intrinsics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ LFORTRAN_API float _lfortran_slog(float x);
LFORTRAN_API double _lfortran_dlog(double x);
LFORTRAN_API float_complex_t _lfortran_clog(float_complex_t x);
LFORTRAN_API double_complex_t _lfortran_zlog(double_complex_t x);
LFORTRAN_API float _lfortran_slog1p(float x);
LFORTRAN_API double _lfortran_dlog1p(double x);
LFORTRAN_API float _lfortran_serf(float x);
LFORTRAN_API double _lfortran_derf(double x);
LFORTRAN_API float _lfortran_serfc(float x);
Expand Down
19 changes: 18 additions & 1 deletion src/lpython/semantics/python_ast_to_asr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4864,13 +4864,30 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> {
}
} else if (ASR::is_a<ASR::Module_t>(*t)) {
ASR::Module_t *m = ASR::down_cast<ASR::Module_t>(t);
std::string sym_name = value + "@" + x.m_attr;
std::string sym_name = value + "_" + x.m_attr + "_external__";
ASR::symbol_t *sym = current_scope->resolve_symbol(sym_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we insert this in the global scope itself?

Copy link
Collaborator Author

@czgdp1807 czgdp1807 Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed IMO. Otherwise it would get declared in global scope in the C code as well and will mix things up with other scopes.

if (!sym) {
sym = import_from_module(al, m, current_scope, value,
x.m_attr, sym_name, x.base.base.loc, false);
LFORTRAN_ASSERT(ASR::is_a<ASR::ExternalSymbol_t>(*sym));
current_scope->add_symbol(sym_name, sym);
} else {
if( ASR::is_a<ASR::ExternalSymbol_t>(*sym) ) {
ASR::ExternalSymbol_t* ext_sym = ASR::down_cast<ASR::ExternalSymbol_t>(sym);
if( ext_sym->m_external != m->m_symtab->resolve_symbol(std::string(x.m_attr)) ) {
sym_name = current_scope->get_unique_name(sym_name);
sym = import_from_module(al, m, current_scope, value,
x.m_attr, sym_name, x.base.base.loc, false);
LFORTRAN_ASSERT(ASR::is_a<ASR::ExternalSymbol_t>(*sym));
current_scope->add_symbol(sym_name, sym);
}
} else {
sym_name = current_scope->get_unique_name(sym_name);
sym = import_from_module(al, m, current_scope, value,
x.m_attr, sym_name, x.base.base.loc, false);
LFORTRAN_ASSERT(ASR::is_a<ASR::ExternalSymbol_t>(*sym));
current_scope->add_symbol(sym_name, sym);
}
}
tmp = ASR::make_Var_t(al, x.base.base.loc, sym);
} else {
Expand Down
37 changes: 34 additions & 3 deletions src/runtime/math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,10 +563,19 @@ def _lfortran_dtan(x: f64) -> f64:
def tan(x: f64) -> f64:
return _lfortran_dtan(x)

@ccall
def _lfortran_slog(x: f32) -> f32:
pass

@overload
def log(x: f32) -> f32:
return _lfortran_slog(x)

@ccall
def _lfortran_dlog(x: f64) -> f64:
pass

@overload
def log(x: f64) -> f64:
return _lfortran_dlog(x)

Expand Down Expand Up @@ -682,21 +691,43 @@ def atanh(x: f64) -> f64:
def expm1(x: f64) -> f64:
return exp(x) - 1.0

@ccall
def _lfortran_slog1p(x: f32) -> f32:
pass

@overload
def log1p(x: f32) -> f32:
return _lfortran_slog1p(x)

@ccall
def _lfortran_dlog1p(x: f64) -> f64:
pass

@overload
def log1p(x: f64) -> f64:
return log(1.0 + x)
return _lfortran_dlog1p(x)


@ccall
def _lfortran_dfmod(x: f64, y: f64) -> f64:
pass

@overload
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of hacky and shouldn't we use some name tweaks at the ASR level?

Copy link
Collaborator Author

@czgdp1807 czgdp1807 Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess overload just does that. We can do it by hand but that will be needed only for fmod and log and nothing else. So ASR name tweaks will be hackish as well. The problem is that the log which is called in lfortran_intrinsics.h is defined in our generated C code as well and hence gets linked to our definition instead of the one in cmath. That’s what causes infinite call loop leading to segmentation fault. Now overload automatically does name mangling and hence linker correctly links because now log is not present instead it is present as a mangled name. Doing the same mangling by hand in ASR is more hackish. Also overloaded log/fmod are harmless as far as I can see.

Copy link
Collaborator Author

@czgdp1807 czgdp1807 Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way is to do it for every imported symbol. For example, from math import log, fabs should be stored as math_log__external_, math_fabs__external_. Then it will be uniform for every imported symbol, so no hacks then. However, I am not sure that's worth the effort as compared to harmless overload decorators in this PR. @certik Do you have any other approach in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way is to do it for every imported symbol. For example, from math import log, fabs should be stored as math_log__external_, math_fabs__external_. Then it will be uniform for every imported symbol, so no hacks then

This sounds better to me. The reason is that the users might not be aware of this and may end up writing a function without an overload decorator (and it's quite intuitive not to use overload for a single function definition.) We need to figure out a generalized approach to handle this and the above might be the one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a more sensible change (by implementing log for f32, fmod for (f32, f32) and log1p for f32). So the overloads make sense now.

The other way is to do it for every imported symbol. For example, from math import log, fabs should be stored as math_log__external_, math_fabs__external_.

This is not possible actually because call names won't resolve correctly. This is a problem on our compiler's end and I fixed it by implementing, log and fmod correctly.

def fmod(x: f64, y: f64) -> f64:
if y == 0.0:
raise ValueError('math domain error')
return _lfortran_dfmod(x, y)


@ccall
def _lfortran_dfmod(x: f64, y: f64) -> f64:
def _lfortran_sfmod(x: f32, y: f32) -> f32:
pass

@overload
def fmod(x: f32, y: f32) -> f32:
if y == f32(0.0):
raise ValueError('math domain error')
return _lfortran_sfmod(x, y)


def remainder(x: f64, y: f64) -> f64:
q: i64
Expand Down