Michael Ho 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: (2 comments) http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/codegen/gen_ir_descriptions.py File be/src/codegen/gen_ir_descriptions.py: http://gerrit.cloudera.org:8080/#/c/10680/3/be/src/codegen/gen_ir_descriptions.py@203 PS3, Line 203: ["COMPARE_INTERPRETED", : "_ZNK6impala18TupleRowComparator18CompareInterpretedEPKNS_8TupleRowES3_"] It seems odd that we are including this interpreted code into cross-compilation only to use it for replacement. Is there a reason the typical approach of matching against the function name not working or is it somehow not robust enough ? 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 doesn't seem easy to maintain in its current form. For the TopNNode, it may be worth exploring whether using the constant argument approach is feasible. For the sorter and the rest, we should be able to achieve what we want to do with the normal clone and replacement albeit a bit tedious. If we decide to go down this route in the long run, I think it warrants more discussion (e.g. can it inadvertently clone a large set of functions which we didn't anticipate because we automate it with this algorithm here now ?) Also, the algorithm implemented probably needs to be validated with functions which exhibit different types of call graphs to validate its behavior. Most importantly, it needs more documentation (e.g. how does it work in general, and why DFS doesn't work etc) and it can be broken down into smaller functions. With all that said, I am not sure we need to implement this algorithm in this patch. -- 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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Comment-Date: Thu, 14 Jun 2018 21:46:52 +0000 Gerrit-HasComments: Yes
