Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/24940 )
Change subject: IMPALA-11917: Upgrade to GCC 15 and LLVM 22 ...................................................................... Patch Set 16: (11 comments) http://gerrit.cloudera.org:8080/#/c/24940/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24940/8//COMMIT_MSG@56 PS8, Line 56: > thx, more optimizations can be investigated after this is merged Ack http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/codegen-anyval.cc@97 PS15, Line 97: : llvm::Type* CodegenAnyVal::GetUnloweredType(LlvmCodeGen* cg, const ColumnType& type) { : llvm::Type* result; : switch(type.type) { : > Wouldn't it be clearer to remove this (+GetUnloweredPtrType and GetAnyValPt Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen-test.cc@639 PS15, Line 639: llvm::StructType* type = llvm::StructType::getTypeByName(codegen->context(), name); > CollectionValue could be added as it has special handling Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.h@101 PS15, Line 101: uto* load > It seems clearer to me to use different name instead of overloads to make i I was trying to limit how many places needed to change, but since we pass in ColumnType I guess it doesn't actually help. I've renamed them. http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.h@102 PS15, Line 102: pe == TYPE_DECIMAL && col_type.GetByte > Is this correct? If I understand correctly the issue is about DecimalVal, n There are two different issues, and I've cleaned up some of the misleading comments. DecimalVal is sometimes allocated from MemPool, which uses 8-byte alignment. Several other operations operate on i128 decimals in tuple memory, which are packed (1-byte alignment). http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.h@310 PS15, Line 310: llvm::ArrayRef<llvm::Constant*> ir_constants, const std::string& name); : : /// Returns reference to llvm context object. Each LlvmCodeGen has its own : /// context to allow multiple threads to be calling into llvm at the same time. : llvm::LLVMContext& context() { return *context_.get(); } > Remove these? Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.h@577 PS15, Line 577: : /// Codegen at the current builder location in function 'fn' to store the : /// max/min('src', value in 'dst_slot_ptr') in 'dst_slot_ptr' : void CodegenMinMax(LlvmBuilder* builder, const ColumnType& type, : llvm::Value* dst_slot_ptr, llvm::Value* src, bool min, llvm::Function* fn); : : /// Codegen to call llvm memcpy intrinsic at the curren > Remove these? Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.cc@445 PS15, Line 445: > This looks a bit odd - can't we ensure that it is always emitted? I didn't find a way to make sure it was always emitted. Open to suggestions. Moved them to helper functions. http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/codegen/llvm-codegen.cc@643 PS15, Line 643: etString(conte > Why not replace call sites with GetPtrType? Same for GetNamedPtrType. Or bu Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/exec/filter-context.cc File be/src/exec/filter-context.cc: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/exec/filter-context.cc@422 PS15, Line 422: gen->GetFunction( > This looks noop now - CreateStructGEP returns a pointer to a member, which Done http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/24940/15/be/src/udf/udf.h@735 PS15, Line 735: a DecimalVal can be embedded in UDA state : // structs allocated from 8-byte alig > This sounds a bit wrong to me, the whole DecimalVals don't live in slots, a Done -- To view, visit http://gerrit.cloudera.org:8080/24940 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7dda730fa98ebe3825969265627a560b0c3095f9 Gerrit-Change-Number: 24940 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Fri, 02 Oct 2026 20:09:19 +0000 Gerrit-HasComments: Yes
