Tim Armstrong has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/3401/5/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 767: char ConstantReplacementPass::pass_id_ = 0; > I think you can make this a constexpr and do in-class initialization For some reason the LLVM API wants this passed by reference, which prevents doing that (we need the char in a byte of memory somewhere, and I guess constexpr doesn't do that). http://llvm.org/docs/doxygen/html/classllvm_1_1FunctionPass.html http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 599: } else { > This looks unchanged to me in PS5. Do you mean "Done" as in "Considered; de Ah, I made the change in the function below but not here. The previous code was safe, since it took the else branch anyway, but had an unnecessary level of nesting there. http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/util/cpu-info.cc File be/src/util/cpu-info.cc: PS4, Line 202: > Can you elaborate to me on why a static initializer would be very unlikely We do it much later in the process lifecycle (well after the process is set up and accepting queries). Aside from that, if a different static initialiser was doing codegen and assuming a bunch of static state was initialised, it would be the one violating the "don't do anything non-trivial in static initialisers" rule. There's a bunch of LLVM global state that needs to be initialised before doing any codegen (see be/src/codegen/llvm-codegen.cc:104) so you'd really have to go to great lengths to do it. -- To view, visit http://gerrit.cloudera.org:8080/3401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ba029ed8589698eb15dbfb0a20dd2a7ea752635 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
