Michael Ho has posted comments on this change. Change subject: IMPALA-3115: Hoist some variables out of loops in cross-compiled code. ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/2661/3//COMMIT_MSG Commit Message: Line 14: Apparently, specifying the : no alias hint isn't sufficient for clang to hoist the multiplication out : of the loop. > it should be able to reduce multiplication to addition. It doesn't seem to be able to do so for all functions. For instance, TupleRow* in_row = in_batch->GetRow(i); isn't converted to addition in ProcessBatchStreaming(). The following is the IR (it's a bit hard to read) but note the multiplication (register %24) at the beginning of the loop body: %i.023 = phi i32 [ 0, %.lr.ph24 ], [ %151, %_ZNK6impala12HashTableCtx16EvalAndHashProbeEPNS_8TupleRowEPj.exit.thread ] %22 = phi %"class.impala::Tuple"** [ %14, %.lr.ph24 ], [ %150, %_ZNK6impala12HashTableCtx16EvalAndHashProbeEPNS_8TupleRowEPj.exit.thread ] %sunkaddr = ptrtoint %"class.impala::RowBatch"* %in_batch to i64 %sunkaddr33 = add i64 %sunkaddr, 24 %sunkaddr34 = inttoptr i64 %sunkaddr33 to %"class.impala::Tuple"*** %23 = load %"class.impala::Tuple"*** %sunkaddr34, align 8, !tbaa !0 %24 = mul nsw i32 %.pre, %i.023 <<<---- %25 = sext i32 %24 to i64 %26 = getelementptr inbounds %"class.impala::Tuple"** %23, i64 %25 %cast_row_ptr.i23 = load %"class.impala::Tuple"** %26, align 8 %tuple_ptr.i35 = bitcast %"class.impala::Tuple"* %cast_row_ptr.i23 to i8* %null_byte.i = load i8* %tuple_ptr.i35, align 1 %null_byte_set.i = and i8 %null_byte.i, 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 http://gerrit.cloudera.org:8080/#/c/2661/3/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 145: zero capacity, Next() will always return NULL > won't it also crash with divide by zero in debug builds? Good point. DCHECK fixed. -- 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: 3 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
