-
Notifications
You must be signed in to change notification settings - Fork 164
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.
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
Improve performance of exp() and exp2() #1712
Conversation
Cannot figure out whats causing the segfault. It does not give any error locally on my linux system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this I would recommend to implement exp
using IntrinsicFunction
API as we have done for abs
, sin
, cos
already. Then you can implement generate_Exp
(similarly as generate_ListIndex
) in visit_IntrinsicFunction
method in asr_to_llvm.cpp
. Same for C backend. Then we won't have to use lfortran_intrinsics.c
runtime library. Each backend will be able to use its preferred way of implementing exponentiation.
References,
For IntrinsicFunction
- https://github.com/lcompilers/lpython/blob/main/src/libasr/pass/intrinsic_function_registry.h (see ListIndex
specifically as we will be implementing exp
in the backend as we have done for list.index
).
lpython/src/libasr/codegen/asr_to_llvm.cpp
Lines 1934 to 1971 in 57ebbc8
void generate_ListIndex(ASR::expr_t* m_arg, ASR::expr_t* m_ele) { | |
ASR::ttype_t* asr_el_type = ASRUtils::get_contained_type(ASRUtils::expr_type(m_arg)); | |
int64_t ptr_loads_copy = ptr_loads; | |
ptr_loads = 0; | |
this->visit_expr(*m_arg); | |
llvm::Value* plist = tmp; | |
ptr_loads = !LLVM::is_llvm_struct(asr_el_type); | |
this->visit_expr_wrapper(m_ele, true); | |
ptr_loads = ptr_loads_copy; | |
llvm::Value *item = tmp; | |
tmp = list_api->index(plist, item, asr_el_type, *module); | |
} | |
void visit_IntrinsicFunction(const ASR::IntrinsicFunction_t& x) { | |
switch (static_cast<ASRUtils::IntrinsicFunctions>(x.m_intrinsic_id)) { | |
case ASRUtils::IntrinsicFunctions::ListIndex: { | |
switch (x.m_overload_id) { | |
case 0: { | |
ASR::expr_t* m_arg = x.m_args[0]; | |
ASR::expr_t* m_ele = x.m_args[1]; | |
generate_ListIndex(m_arg, m_ele); | |
break ; | |
} | |
default: { | |
throw CodeGenError("list.index only accepts one argument", | |
x.base.base.loc); | |
} | |
} | |
break ; | |
} | |
default: { | |
throw CodeGenError( ASRUtils::IntrinsicFunctionRegistry:: | |
get_intrinsic_function_name(x.m_intrinsic_id) + | |
" is not implemented by LLVM backend.", x.base.base.loc); | |
} | |
} | |
} |
https://llvm.org/docs/LangRef.html#llvm-exp-intrinsic, https://llvm.org/docs/LangRef.html#llvm-exp2-intrinsic, https://llvm.org/doxygen/IRBuilder_8cpp_source.html#l00956
Something like,
builder->CreateUnaryIntrinsic(llvm::Intrinsic::exp, tmp)
exp and exp2 should be implemented using the IntrinsicFunction mechanism. |
@certik I just started working on it right now, that was just a merge commit. I'll implement them now. |
Implemented # no import
def test():
x : f64 = 0.5
print(exp(x))
print(exp2(x))
test() (lp) sarthak@pop-os:~/lpython$ src/bin/lpython --fast examples/test.py
1.64872127070012819e+00
1.41421356237309515e+00 |
One thing I observed after implementing them is that Python does not actually have Just a thought - since there is repetition of code in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good. Yes, we should simplify using macros, but in subsequent PRs.
If we do from math import exp
print(exp(0.5)) or do import math
print(math.exp(0.5)) that still uses the old implementation. {"math_exp", {&Exp::create_Exp, &Exp::eval_Exp}}, In def exp(x: f64) -> f64:
"""
Return `e` raised to the power `x`.
"""
return math_exp(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While doing AST to ASR transition, we also need to create IntrinsicFunction
node in ASR as well. See,
lpython/src/lpython/semantics/python_ast_to_asr.cpp
Lines 6446 to 6460 in b28a440
if (!s) { | |
std::set<std::string> not_cpython_builtin = { | |
"sin", "cos", "gamma", "tan", "asin", "acos", "atan" | |
}; | |
if (ASRUtils::IntrinsicFunctionRegistry::is_intrinsic_function(call_name) | |
&& not_cpython_builtin.find(call_name) == not_cpython_builtin.end()) { | |
ASRUtils::create_intrinsic_function create_func = | |
ASRUtils::IntrinsicFunctionRegistry::get_create_function(call_name); | |
Vec<ASR::expr_t*> args_; args_.reserve(al, x.n_args); | |
visit_expr_list(x.m_args, x.n_args, args_); | |
tmp = create_func(al, x.base.base.loc, args_, | |
[&](const std::string &msg, const Location &loc) { | |
throw SemanticError(msg, loc); }); | |
return ; | |
} else if (intrinsic_procedures.is_intrinsic(call_name)) { |
Probably adding "exp" should work. Also, we should raise an error if from math import exp
or from numpy import exp
isn't there. For "exp2" we should raise an error if from numpy import exp2
is absent. Also math.exp
should only accept scalars (error otherwise). numpy.exp
and numpy.exp2
can accept arrays.
@czgdp1807 So I did this std::set<std::string> not_cpython_builtin = {
"sin", "cos", "gamma", "tan", "asin", "acos", "atan", "exp", "exp2"
}; and it raised the error as expected. semantic error: Function 'exp' is not declared and not intrinsic
--> examples/hi.py:12:7
|
12 | print(exp(0.90))
| ^^^^^^^^^ But the problem is that when we do import, it is using the old implementation inside |
Well try to fix this. Remove Python implementations of |
@czgdp1807 Please check if this is the correct way to handle this.
|
We already have the support for that. See, lpython/src/libasr/pass/array_op.cpp Lines 674 to 758 in baf30f7
And I think we should have an elemental flag in For now add your function below, lpython/src/libasr/pass/intrinsic_function_registry.h Lines 932 to 939 in baf30f7
|
@czgdp1807 I already tried that, did not work. LLVM throws a codegen error. I think its because other functions like code generation error: asr_to_llvm: module failed verification. Error:
Intrinsic has incorrect return type!
double* (double*)* @llvm.exp.p0f64
FPExt only operates on FP
%58 = fpext double* %57 to double If we do code generation error: asr_to_llvm: module failed verification. Error:
Intrinsic has incorrect return type!
double* (double*)* @llvm.exp.p0f64
Stored value type does not match pointer operand type!
store double* %108, double* %92, align 8
double |
I don't see either lpython/src/libasr/pass/intrinsic_function_registry.h Lines 1007 to 1014 in f4a0a6d
So probably its related to incorrect processing of |
@Thirumalai-Shaktivel please review my approach to solving the problem mentioned in #1712 (comment) . How do I fix the error in macos CI test? |
LGTM! |
Just do what the error is saying:
I think we need to inlude |
I believe this PR is ready. Let me know if there's anything else that needs to be done.
Edit: |
ASRUtils::create_intrinsic_function create_func = | ||
ASRUtils::IntrinsicFunctionRegistry::get_create_function(call_name); | ||
Vec<ASR::expr_t*> args_; args_.reserve(al, x.n_args); | ||
visit_expr_list(x.m_args, x.n_args, args_); | ||
if (ASRUtils::is_array(ASRUtils::expr_type(args_[0])) && | ||
current_scope->resolve_symbol("numpy") == nullptr ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be the wrong check, I think it doesn't matter if import numpy
is present, but rather if exp
is imported from numpy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following case,
- The user hasn't installed
numpy
from pip, conda or anywhere on the internet. - They have written a
numpy
package locally with their own API. - Now they do
from numpy import exp
(exp
is defined by them in theirnumpy
package). - So will the
ASR::IntrinsicFunction
will be used or theirexp
will be used. - I think we should test this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czgdp1807 we already have this issue with sin
and other functions I think, so we can probably fix this in a separate PR.
Thanks, I think it looks pretty good overall, there are just some smaller things to resolve. |
Changes:
Regarding #1712 (comment), right now I could not think of a way to solve this, but as Ondrej said, we can tackle this in another issue/PR. |
LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for #1673 to be merged.
No description provided.