Michael Ho has posted comments on this change. Change subject: IMPALA-3115: Hoist some variables out of loops in cross-compiled code. ......................................................................
Patch Set 7: (12 comments) I checked the IR and they are inlined and row_ is definitely in LLVM register. I didn't check the final machine code generated as it's a bit painful and I don't know if there is a way to tell LLVM what to spill to stack if it's not in register somehow. Code snippet from PartitionedAggregationNode::ProcessBatch(): %18 = bitcast %"class.impala::RowBatch"* %batch to i32* <<--- num_rows %19 = getelementptr inbounds %"class.impala::RowBatch"* %batch, i64 0, i32 3 %20 = load i32* %19, align 4, !tbaa !5 %21 = sext i32 %20 to i64 %22 = getelementptr inbounds %"class.impala::RowBatch"* %batch, i64 0, i32 5 %23 = load %"class.impala::Tuple"*** %22, align 8, !tbaa !0 %24 = load i32* %18, align 4, !tbaa !5 %25 = mul nsw i32 %24, %20 <<--- num_rows * num_tuples_per_row %26 = sext i32 %25 to i64 %27 = getelementptr inbounds %"class.impala::Tuple"** %23, i64 %26 %28 = icmp sgt i32 %25, 0 br i1 %28, label %.lr.ph.i, label %._crit_edge.i .lr.ph.i: ; preds = %_ZN6impala6StatusD1Ev.exit9.i %29 = getelementptr inbounds %"class.impala::PartitionedAggregationNode"* %this, i64 0, i32 1 br label %30 ; <label>:30 ; preds = %_ZN6impala6StatusD1Ev.exit11.i, %.lr.ph.i %row.024.in.i = phi %"class.impala::Tuple"** [ %23, %.lr.ph.i ], [ %272, %_ZN6impala6StatusD1Ev.exit11.i ] <<<--- 'row' in the loop %cast_row_ptr.i63 = load %"class.impala::Tuple"** %row.024.in.i, align 8 %tuple_ptr.i130 = bitcast %"class.impala::Tuple"* %cast_row_ptr.i63 to i8* %null_byte.i10 = load i8* %tuple_ptr.i130, align 1 %null_byte_set.i = and i8 %null_byte.i10, 1 %slot_is_null.i = icmp eq i8 %null_byte_set.i, 0 br i1 %slot_is_null.i, label %not_null.i, label %null.i ... ... _ZN6impala6StatusD1Ev.exit11.i: ; preds = %39, %src_not_null.i, %src_not_null.i42, %_ZN6impala26PartitionedAggregationNode10ProcessRowILb0EEENS_6StatusEPNS_8TupleRowEPKNS_12HashTableCtxE.exit.i %272 = getelementptr inbounds %"class.impala::Tuple"** %row.024.in.i, i64 %21 <<--- Next() of iterator %273 = icmp ult %"class.impala::Tuple"** %272, %27 br i1 %273, label %30, label %._crit_edge.i http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 172: out_row > or just use out_batch_iterator.Get() here, but okay to ignore. Done http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 50: out_row > or just use out_batch_iterator.Get() in place of this variable, but okay to Will keep 'out_row' for less verbose code but I see your point. Hopefully, other people modifying this code will pay attention to the existing pattern. Line 52: RowBatch::Iterator probe_batch_iterator(probe_batch_.get(), probe_batch_pos_); > Maybe comment that this iterator corresponds to probe_batch_pos_ which is a Done http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 1024: out_row > consider getting rid of this. might help register pressure assuming to allo Done http://gerrit.cloudera.org:8080/#/c/2661/7/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 149: Last > Pointer to the row after the Batch::num_rows() row, for read iterators. Done. I used 'the row after the last row' instead. Line 150: RowBatch* const parent_; > we generally put the fields after the methods. also, add brief comments bef Sorry, will pay more attention in the future. Line 153: /// An iterator for going through a row batch, starting at 'row_idx'. > this should be the class comment. Done Line 157: num_tuples_per_row_ * parent->num_rows_ > reverse this or previous line to make them consistent. Done Line 171: DCHECK > DCHECK_LE Done Line 172: reinterpret_cast<TupleRow*>(row_); > could be: Get(); Done Line 175: /// Useful for read iterators to determine its limit. > Returns true once the iterator has traversed 'RowBatch::num_rows() - row_id Done. Slightly reworded it. Line 387: /// Macro for iterating through '_row_batch', starting at '_start_row_idx'. > document _row: '_row' is a TupleRow* for the current row, or something. Done -- To view, visit http://gerrit.cloudera.org:8080/2661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7152b1fb094b3c3574d203e3774f4297f2225dc Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
