Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option ...................................................................... Patch Set 7: Code-Review+1 (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 TCodeGenOptLevel range? Because we will reset the cache entry from existing cache entry in the look up process, maybe some abnormal could be detected earlier here if opt value goes wrong. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@183 PS7, Line 183: num_opt_functions nit. could you please add comments for the new fields? Would be good to tell the difference between the existing num_functions and num_instructions. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@189 PS7, Line 189: TCodeGenOptLevel::type opt_level; nit. could you please add a simple comment for the field. 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: break; nit. do we need the default case to handle unexpected values? -- 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: 7 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 17:51:49 +0000 Gerrit-HasComments: Yes
