Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10680 )
Change subject: IMPALA-3816, IMPALA-4065: Remove the indirection to TupleRowComparator::Compare() ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/codegen/llvm-codegen.cc@915 PS3, Line 915: llvm::Function* LlvmCodeGen::ReplaceCallSitesRecursively(llvm::Function* caller, > As discussed offline, this is an interesting idea. On the other hand, this This patch is too big for me to adopt at the moment but it would be nice to move it along at some point. I think doing something like this makes sense to solve two problems that we don't currently handle in codegen: 1. recursive functions (like in Sorter) where we need to fix up the specialised function to call itself (directly or indirectly) 2. code that we want to specialise where the function we need to replace is called from non-Impala code (like with std::push_heap and std::pop_heap in Top N node) A nice side-effect would be to avoid having to sprinkle IR_ALWAYS_INLINE in as many places. So I like having a function that achieves this. Agree that it needs a little thought about how to avoid silent regressions (either cloning too many functions or not enough - e.g. if a virtual function call was introduced by a code change, preventing this algorithm from following the callchain). We could potentially return the number of functions cloned and have DCHECKs in callers on that. I also think, per your comment, that we don't need to find the SCC to solve this. If we're planning to specialise a set of functions F, with entry point function e, then we need to find F', the set of functions reachable from e that call a function in F directly or indirectly. Once you have that, you clone all those functions and do a pass over them to replace all calls to the original versions with the specialised versions. This algorithm does the replacement more incrementally, doing the replacement a SCC at a time. I don't see a real gain from doing it this way (cache locality, maybe?). I think you can indeed find F' with plain DFS. DFS will find you the subgraph reachable from e, and you need to augment it to keep track of the set of nodes that call one of the specialised functions directly/indirectly. That seems straightforward - you add a node to the set every time you follow an edge that leads to a node in the visit. http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/codegen/llvm-codegen.cc@981 PS3, Line 981: } else if (llvm::isa<llvm::InvokeInst>(&*dfs_stack.top().iter)) { We generally want to avoid Invoke instructions in codegen'd code - those are cases where clang things the callee may throw and exception. http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/exec/topn-node.cc@123 PS3, Line 123: codegen->GetFunction(IRFunction::COMPARE_INTERPRETED, false), tuple_compare_fn); This is weird because it doesn't remove the branch in Compare() on codegend_compare_fn_ == NULL. -- To view, visit http://gerrit.cloudera.org:8080/10680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4657ac09daf20408797856d94521d417d8cf171 Gerrit-Change-Number: 10680 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Mar 2019 19:16:44 +0000 Gerrit-HasComments: Yes
