Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@178 PS7, Line 178: opt_level > nit. do you think it good to add a DCHECK for opt_level to see if it out of Compilation mostly handles this, and this value isn't directly used for any array accesses. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@183 PS7, Line 183: er of instruction > nit. could you please add comments for the new fields? Would be good to tel Done http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@189 PS7, Line 189: /// The hashcode of function name > nit. could you please add a simple comment for the field. Done http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc@1454 PS7, Line 1454: opt = llvm::PassBuilder::OptimizationLevel::O3; > nit. do we need the default case to handle unexpected values? The comment at line 1438 covers that. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 12 Sep 2023 18:35:23 +0000 Gerrit-HasComments: Yes
