[bugfix] fix the undeclared identifier 'f'#14879
[bugfix] fix the undeclared identifier 'f'#14879quic-sanirudh merged 2 commits intoapache:mainfrom jikechao:fix_undefined_f
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
|
@tvm-bot rerun |
| // Check if it is available in global function table as injected function. | ||
|
|
||
| llvm::Function* f = module_->getFunction(MakeStringRef(global_symbol)); | ||
| auto callee = [&]() -> llvm::Value* { |
There was a problem hiding this comment.
callee is only used when #if TVM_LLVM_VERSION >= 90 is true. So, could we just move the whole callee initialization lambda into that branch of the conditional directive?
There was a problem hiding this comment.
The callee was intended to be used in both cases, with the #if normalizing callee into either a llvm::Value* or a llvm::FunctionCallee depending on the LLVM version. I think we should keep the definition of callee outside the conditional, but should replace auto ext_callee = f; with auto ext_callee = callee;. That way, callee is initialized as appropriate module/context/external function pointer in both branches, and is then converted to the type required by the LLVM API inside the directive.
There was a problem hiding this comment.
That makes more sense, thanks.
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Thank you for the catch! I think we could have a smaller fix within the ifdef, which would also avoid needing to expose the definition of f to the larger scope.
| llvm::FunctionType* ftype = llvm::FunctionType::get(GetLLVMType(ret_type), arg_types, false); | ||
| // Check if it is available in global function table as injected function. | ||
|
|
||
| llvm::Function* f = module_->getFunction(MakeStringRef(global_symbol)); |
There was a problem hiding this comment.
Instead of exposing llvm::Function* f outside the scope, could we instead replace auto ext_callee = f; on line 490 with auto ext_callee = callee;? The current change would allow compilation, but would skip the optional pointer-cast of the callee.
There was a problem hiding this comment.
Or, alternatively, I think we could simplify it by returning builder_->CreateCall(ftype, callee, arg_values);. This method is available as far back as 3.7.0 (commit link), and since TVM requires LLVM 4.0 or higher, we would avoid needing the #if directive altogether.
There was a problem hiding this comment.
@Lunderberg @quic-sanirudh Thanks for your guidance. I have corrected the patch, could you recheck the code change again?
junrushao
left a comment
junrushao
left a comment
There was a problem hiding this comment.
Thanks for fixing the legacy issue!
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Thank you for the changes, and looks good to me!
quic-sanirudh
left a comment
quic-sanirudh
left a comment
There was a problem hiding this comment.
LGTM, Thanks. Guess we can merge this now as Eric has already approved it.
* fix the undeclared identifier 'f' * Update codegen_cpu.cc
5 participants
This pr fixed the source code build problem found in issue #14878
The variable 'f' is a local variable that is inviable for the outside code. This pr moved the 'f' definition to the outside of the condition and fix the crash.
cc @Lunderberg @kparzysz-quic