Enable C test for test_math.py
#1357
Conversation
| @@ -4742,13 +4742,30 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> { | |||
| ASRUtils::expr_value(enum_member_variable->m_symbolic_value)); | |||
| } 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); | |||
There was a problem hiding this comment.
Should we insert this in the global scope itself?
There was a problem hiding this comment.
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.
czgdp1807
force-pushed
the
test_math_c
branch
from
0541b1c to
8903eca
Compare
| @@ -687,6 +688,7 @@ def log1p(x: f64) -> f64: | |||
| return log(1.0 + x) | |||
|
|
|||
|
|
|||
| @overload | |||
There was a problem hiding this comment.
This is kind of hacky and shouldn't we use some name tweaks at the ASR level?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The other way is to do it for every imported symbol. For example,
from math import log, fabsshould be stored asmath_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.
There was a problem hiding this comment.
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, fabsshould be stored asmath_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.
| if( v->m_symbolic_value ) { | ||
| this->visit_expr(*v->m_symbolic_value); | ||
| decl += indent + "#define " + std::string(v_ext->m_name) + " " + src + "\n"; | ||
| src.clear(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would not use C macros.
So constant C variables should work for this case?
There was a problem hiding this comment.
Either that, or we can initialize the global variables when the program starts. I think both seems better than macros.
czgdp1807
force-pushed
the
test_math_c
branch
from
864a4e3 to
f1b31d8
Compare
|
This needs some work. There might be good ideas here. |
Closes #1246
Related discussion - #1243 (comment)