Tim Armstrong has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/3401/7/be/src/codegen/constant-replacement.cc File be/src/codegen/constant-replacement.cc: PS7, Line 31: gen, > nit: long line Done http://gerrit.cloudera.org:8080/#/c/3401/7/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: PS7, Line 130: > Can it just be SimpleConstantReplacer ? Done http://gerrit.cloudera.org:8080/#/c/3401/6/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS6, Line 651: symbol_map.find(callee->getName()) != symbol_map.end()) > I am not sure I understand this comment here. This function as it's written I mean if we call ReplaceCallSitesWithConstants() N times and there are M function calls in the IR, only R of which are replaceable, we end up doing N * M virtual calls to TryReplace() instead of N * R. Not the end of the world, but it doesn't make sense to me to rework it to make it (maybe?) slightly simpler but also slightly less efficient. WRT the multiple replacers thing - you're right that right now it doesn't have such an optimisation. But it's nice if the ConstantReplacer interface makes such optimisations possible. http://gerrit.cloudera.org:8080/#/c/3401/7/be/src/exprs/expr-constant-replacement.cc File be/src/exprs/expr-constant-replacement.cc: PS7, Line 81: > Is it possible to do it as a LLVM pass too so we can unify multiple differe I'm not sure exactly what you're proposing. I feel like this is ok as-is since there's not really any duplication of logic. I don't think wrapping the constant replacement in an LLVM pass instead of calling ReplaceCallSitesWithConstants() on its own buys us anything here. Trying to instantiate a single FunctionPass object that handles replacing types in all functions seems complicated since we have different types for each function (and we also need to make sure we do it before inlining any functions). http://gerrit.cloudera.org:8080/#/c/3401/7/be/src/util/cpu-info.cc File be/src/util/cpu-info.cc: PS7, Line 184: class CpuInfoConstantReplacer : public ConstantReplacer > Is it possible to nest this class inside CpuInfo class instead ? Definitely possible but I don't think it works out better: * We couldn't (easily) call it the obvious name of CpuInfo::ConstantReplacer() since the name would then be ambiguous with the parent class. * We need to expose the class's existence in the header file. Currently it's self-contained in this .cc file. -- 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: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
