impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3637,IMPALA-3636: refactor codegen constant replacement
Date Fri, 22 Jul 2016 19:30:35 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message