Dan Hecht has posted comments on this change. Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen ......................................................................
Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/5732/4//COMMIT_MSG Commit Message: Line 50: defined in the boost library. is there some verification code we can add to ensure that this problem won't happen again (or isn't happening elsewhere)? http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS4, Line 326: which are also defined in the main module this doesn't seem to match what the code is doing. Isn't it materializing functions that are referenced by functions of the new module? But why is this needed? why don't they get materialized when we follow the chain of references in MaterializeFunctionHelper()? http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS4, Line 85: their callee functions what is a "callee" for a global variable? do you mean reference? If so, how about: // Map from global variables/functions to the functions that they reference. PS4, Line 86: CalleeMap again, callee sounds to me like only functions would be keys in this map. FunctionRefsMap? PS4, Line 160: f 'eager' is false, the : /// functions in the module are not materialized. this seems to contradict the class header comment that says "llvm::Function objects in the module are materialized lazily". i.e. the behavior you are documented is the expected behavior, no? So are you adding 'eager' so that the default behavior can be overridden? if so, it should say something like: If 'eager' is true, the functions are materalized. Otherwise they are materialized lazily via GetFunction(). And if that's what's going on, why not call the parameter 'materialize"? But, as a caller, how do I choose whether I'm suppose to materialize or not at this point? The need for this seems to contradict the whole explanation about lazy materialization in the header comment. Finally, rather than threading through this parameter, why not just have the caller do module->MaterializeAll() with the returned module? PS4, Line 617: which also need to be materialized if : /// the input global variable or function is materialized is this really specific to materialization? wouldn't it be enough to just say: ... all the functions referenced by it. The materialization code itself can explain (or be self documenting) about why references need to be followed when materializing. PS4, Line 625: fns_to_materialize_ why this name change? does this now store functions that need to be materialized for various reasons? don't we materialize functions that aren't in this set? http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: since this takes so long to execute, should we either move expr-test.cc to run only in exhaustive, or run only codegen tests when exhaustive? -- To view, visit http://gerrit.cloudera.org:8080/5732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
