Michael Ho has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 6: (19 comments) http://gerrit.cloudera.org:8080/#/c/3401/6/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: PS6, Line 35: struct ReplaceableSymbol { I find the entire layout of classes a bit confusing and they can be simplified: ReplaceableSymbol should be merged with SymbolMergeEntry below to form a struct like the following: struct ReplaceableFunction { int constant_arg_index; int num_replaced; Llvm::Constant* replacement_constant; // constructors etc .... }; The map symbol_map_ can be a private field in ConstantReplacer: std::unordered_map<std::string, ReplaceableFunction> fn_map_; In this way, the fn_map_ can contain mappings of functions with and without function arguments to their corresponding replacement constants. ConstantReplacer should contain the function AddReplacement() for callers to insert entries into the fn_map_. In this way, the class SimpleNoArgConstantReplacer doesn't even need to exist. PS6, Line 35: Symbol ReplaceableFunction seems to be a more precise description. PS6, Line 53: a set of symbols. Doesn't it only handle replacement of functions ? "symbols" seems to imply that it could be global variables too. PS6, Line 54: This uses the "Visitor" pattern, IMHO, this is not important details. It would be more helpful to explain when to call which functions and provide example usage. Line 55: /// to replace a function callsite with a symbol. I think this class warrants a more detailed explanation. Essentially, the replacer is used for replacing various call instructions with constants. It's important to point out how a symbol to LLVM constant mapping can be established and which function to call to replace call instructions with constant. PS6, Line 59: virtual std::vector<ReplaceableSymbol> HandledSymbols() = 0; This seems to be implementation details which really shouldn't be exposed to callers. PS6, Line 62: 'constant_arg_index' of -1 is found. It may be easier to understand to say the replaced function has no argument instead of constant_arg_index' is -1 ? PS6, Line 65: virtual llvm::Value* ReplaceNoArgs( Following from the restructuring comments above, I don't see there is need to expose ReplaceNoArgs() and ReplaceOneArg() as public functions. In essence, the ConstantReplacer::TryReplace() should just be a mechanical look up of replacement constant based on the input symbol using the symbol_map_; This seems unnecessarily overcomplicated. Not sure if there are certain other use cases you have in mind ? PS6, Line 70: /// This function is invoked when a call to one of the handled symbols with : /// a non-negative 'constant_arg_index' is found. This function handles replacing functions with at least one constant argument with a constant. PS6, Line 76: int64_t arg) Please document the arguments. PS6, Line 89: class SimpleNoArgConstantReplacer Please see comments above. This class may not need to exist. PS6, Line 91: a replacement Add a replacement entry (i.e. function name 'symbol' and its replacement constant value 'replacement) to 'symbol_map_'. PS6, Line 92: void AddReplacement(const std::string& symbol, llvm::Value* replacement) { Please see comments above. This may fit in ConstantReplacer better. PS6, Line 120: struct SymbolMapEntry { Please see comments above. 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()) Is it really necessary to expose the symbol_map to the caller ? Isn't it implementation detail of the Replacer itself ? Since the replacer has the list of symbols which it can replace, TryReplace() can simply take a CallInst* as argument. Then the loop can simply be: for (inst_iterator: ...) { if (CallInst::classof(inst)) { replaced += replacer->TryReplace(this, call_instr); } } http://gerrit.cloudera.org:8080/#/c/3401/6/be/src/exec/hash-table-constant-replacer.h File be/src/exec/hash-table-constant-replacer.h: PS6, Line 17: : #ifndef IMPALA_EXEC_HASH_TABLE_CONSTANT_REPLACER_H : #define IMPALA_EXEC_HASH_TABLE_CONSTANT_REPLACER_H Shouldn't these lines be above the #include lines above ? http://gerrit.cloudera.org:8080/#/c/3401/6/be/src/exprs/expr.cc File be/src/exprs/expr.cc: PS6, Line 580: class ArgTypeConstantReplacer : public ConstantReplacer This seems to be applicable to Decimal only so shouldn't this be moved to decimal.cc or decimal-replacer.cc ? PS6, Line 609: rn codegen->GetIntConstant(TYPE_INT, type.precision); Please see comments for the ConstantReplacer class. I don't see why this cannot simply be an entry in the symbol_map_ inserted by AddReplacement(). PS6, Line 616: const FunctionContext::TypeDesc& return_type_; : const vector<FunctionContext::TypeDesc>& arg_types_; If we can insert all the mappings from function/symbol name to replacement constant in the constructor, we may not even need to keep these variables around. -- 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
