Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737 PS2, Line 737: enabled_cpu_attrs_ > Thanks for the explanation. I agree that we should be defensive. Maybe all How about we move the method EnableCPUFeature out from this class and into the test class only and maintain this duplication of attributes there. and also comment on cpu_attrs_ that it should not be modified except in InitializeLlvm() or for testing purposes. http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc@1519 PS3, Line 1519: long nit: not a big deal but just to be consistent with CpuInfo and the static flags, use int64_t -- To view, visit http://gerrit.cloudera.org:8080/10979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f Gerrit-Change-Number: 10979 Gerrit-PatchSet: 3 Gerrit-Owner: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 20 Jul 2018 19:10:42 +0000 Gerrit-HasComments: Yes
