Skye Wanderman-Milne has posted comments on this change. Change subject: Specify whether to clone IR function in GetFunction() instead of ReplaceCallSites() ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/2112/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 455: Function* LlvmCodeGen::GetFunction(IRFunction::Type function, bool clone) { > instead of this clone param, why not simply have the callers that need the I made it a required param here so you can't forget to clone the function, or at least have to consider whether you need to clone it. I think otherwise it's too easy to write a new codegen function without realizing you need to clone sometimes. http://gerrit.cloudera.org:8080/#/c/2112/3/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 263: int ReplaceCallSites(llvm::Function* caller, llvm::Function* new_fn, > since new_fn is an in/out param, it should go to the end It's an input-only param, it's a pointer and not a reference since that's how LLVM's APIs work (technically LLVM is possibly altering new_fn under the hood, e.g. to update ref counts, but for our purposes it's pure input). -- To view, visit http://gerrit.cloudera.org:8080/2112 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8eabb5c062ee223c5de9df40aacfdc9dcda5ba63 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
