gemini-code-assist[bot] commented on code in PR #18818:
URL: https://github.com/apache/tvm/pull/18818#discussion_r2852530015
##########
src/target/llvm/llvm_instance.cc:
##########
@@ -1024,7 +1056,21 @@ bool LLVMTarget::ApplyLLVMOptions(bool
apply_otherwise_revert, bool dry_run) {
const Option& new_opt = new_options[i];
const Option& saved_opt = saved_llvm_options_[i];
- llvm::cl::Option* base_op = options[new_opt.name];
+ llvm::cl::Option* base_op = nullptr;
+ auto& options = llvm::cl::getRegisteredOptions();
Review Comment:
Similar to the comment in `LLVMTargetInfo` constructor,
`llvm::cl::getRegisteredOptions()` returns a reference to a static `StringMap`.
Calling it repeatedly inside this loop is inefficient. It should be called once
before the loop and the reference reused.
```c
const auto& new_options = GetCommandLineOptions();
auto& options = llvm::cl::getRegisteredOptions();
for (size_t i = 0, e = saved_llvm_options_.size(); i != e; ++i) {
const Option& new_opt = new_options[i];
const Option& saved_opt = saved_llvm_options_[i];
llvm::cl::Option* base_op = nullptr;
#if TVM_LLVM_VERSION >= 220
auto it = options.find(new_opt.name);
if (it != options.end()) {
base_op = it->second;
}
#else
if (options.count(new_opt.name)) {
base_op = options[new_opt.name];
}
#endif
```
##########
src/target/llvm/llvm_instance.cc:
##########
@@ -233,15 +233,19 @@ LLVMTargetInfo::LLVMTargetInfo(LLVMInstance& instance,
}
if (const auto& v =
Downcast<ffi::Optional<ffi::Array<ffi::String>>>(target.Get("cl-opt"))) {
- llvm::StringMap<llvm::cl::Option*>& options =
llvm::cl::getRegisteredOptions();
bool parse_error = false;
for (const ffi::String& s : v.value()) {
Option opt = ParseOptionString(s);
if (opt.type == Option::OptType::Invalid) {
parse_error = true;
continue;
}
+ auto& options = llvm::cl::getRegisteredOptions();
Review Comment:
The `llvm::cl::getRegisteredOptions()` function returns a reference to a
static `StringMap`. Calling it repeatedly inside the loop, as done here, is
inefficient. It would be better to call it once before the loop and store the
reference, then reuse that reference within the loop. This applies to the
similar change in `ApplyLLVMOptions` as well.
```c
auto& options = llvm::cl::getRegisteredOptions();
bool parse_error = false;
for (const ffi::String& s : v.value()) {
Option opt = ParseOptionString(s);
if (opt.type == Option::OptType::Invalid) {
parse_error = true;
continue;
}
#if TVM_LLVM_VERSION >= 220
if (options.find(opt.name) != options.end()) {
#else
if (options.count(opt.name)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
