Dan Hecht has posted comments on this change. Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen ......................................................................
Patch Set 6: Code-Review+2 (8 comments) Thanks, that's easier to follow. Just some more comment clarification suggestions. http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS5, Line 328: in case they : // are chosen by the linker or referenced by functions in the new module. Note tha > linkModule() will materialize function. Okay. How about clarifying: Note that linkModule() will materialize the functions from the new module. http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 172: RETURN_IF_ERROR(LlvmCodeGen::CreateFromMemory(&init_pool, NULL, "init", &init_codegen)); how about a comment then: // LLVM constructs "uses" lists only when materializing. PS6, Line 195: only this "only" doesn't make sense, because you also include functions that don't have native versions. PS6, Line 195: by delete double "by" PS6, Line 198: refereced spelling and period not comma at end. http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS6, Line 85: global variables'/functions' function names (no longer variables). PS6, Line 146: libbackend is this referring to libfesupport.so? Line 616: /// returns the set of names of all functions referenced by it. Now that this only stores the call graph, how about just explaining it in terms of that: The call-graph for IR functions within the main module. Used to determine dependencies while materializing functions. Or something like that. -- 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: 6 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
