Michael Ho has posted comments on this change. Change subject: IMPALA-3674: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/3220/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS3, Line 212: false > why do we want to eagerly materialize in this case? I was thinking the linker may not work with lazy materialization but looks like it actually works fine (test_udfs.py passes). So switching to lazy materialization for all paths. Line 226: } > it looks like there are only two callers of this, one with false and one tr The eager materialization path has been removed in the new patch so it's irrelevant now. PS3, Line 227: UNLIKELY > nit: this seems unnecessary. i think we should keep UNLIKELY only on perf s Done PS3, Line 299: isMaterializable > what does it mean to not be materializable? If it's only a declaration without a definition (e.g. extern declaration or LLVM intrinsics). Note that we are going through all the functions in the module so it includes more than the cross-compiled functions. Line 547: VLOG_QUERY << "Skipping materialization of " << fn->getName().str(); > how verbose is this logging? Yes, it's a bit verbose. It's removed in the new patch. Line 553: LOG(ERROR) << "Failed to materialize " << fn->getName().str() << ": " << err.message(); > is it okay to continue on this case? when does this happen? we don't need t Good point. We should abort at this point. However, the existing callers of GetFunction() are really bad at handling failures so that requires clean up in another patch. I will return an error status in this function and GetFunction() will return a NULL pointer for now. We should clean up the code in a separate patch to propagate the status all the way back. Line 557: DCHECK(!fn->isMaterializable()); > what does this mean? that it's already been materialized? Yes. LLVM will mark materialized functions as not materializable. Comment added. PS3, Line 574: MaterializeFunction > from the name it was surprising that it materializes all reachable function Done. I call it MaterializeFunctionAndCallee(). http://gerrit.cloudera.org:8080/#/c/3220/3/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS3, Line 249: referenced only by global variables > if referenced by global variables, how do we know they are not reachable? I did an audit of the global variables which contains function pointer and they can all be resolved by external linkage as they are all defined in the impalad binary. They are either boost library functions or cross-compiled functions which aren't directly called (IfExpr::GetBooleanVal). I don't think they are inlined at all so they don't really need to be cross-compiled. There is some room for improvement for the cross-compiled code as some of them are still calling virtual functions so it isn't reaping any benefit from dynamic code generation. They will be addressed in a separate change. Line 328: llvm::Function* GetFunction(const string& symbol); > document this. e.g. does it return a clone or not? Done PS3, Line 433: 'llvm_module_ir'. > Will update the comment in the next patch. Done PS3, Line 434: so callers should call : /// MaterializeFunction() > when should they call that? if they are suppose to call it, why wouldn't Ll Done Line 440: /// file. The caller is responsible for cleaning up module. > let's document that these routines are eagerly materialized and why. It's also using lazy materialization now. Comment fixed. Line 503: void MaterializeFunction(llvm::Function* fn); > what is the difference between these? seems they could use separate commen Done -- To view, visit http://gerrit.cloudera.org:8080/3220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ed7862fc5e86005ecea83fa2ceb489e737d66b2 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
