Impala Public Jenkins has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/10243 )
Change subject: IMPALA-6882: prevent instr. hoist from CpuInfo::IsSupported()
......................................................................
IMPALA-6882: prevent instr. hoist from CpuInfo::IsSupported()
Marking the __asm__ with __volatile__ *should* prevent the compiler from
speculatively executing the instruction before the branch.
Testing:
Added a regression test that tries to emulate the problematic pattern,
but I was unable to reproduce a crash on a system with popcnt support -
there's probably some instruction scheduling differences.
Manually checked that popcnt was inside the expected branch:
(gdb) disassemble /s impala::HdfsScanNodeBase::StopAndFinalizeCounters
...
160 if (LIKELY(CpuInfo::IsSupported(CpuInfo::POPCNT))) {
0x0000000000e7261b <+491>: and $0x10,%esi
0x0000000000e7261e <+494>: je 0xe73031
<impala::HdfsScanNodeBase::StopAndFinalizeCounters()+3073>
be/src/util/sse-util.h:
147 __asm__ __volatile__("popcntq %1, %0" : "=r"(result) : "mr"(a) :
"cc");
0x0000000000e72624 <+500>: popcnt %rdx,%rdx
0x0000000000e72629 <+505>: movslq %edx,%rsi
...
Change-Id: I9ec51bdfcb9455c93ff69827929a59fcccb81b80
Reviewed-on: http://gerrit.cloudera.org:8080/10243
Reviewed-by: Jim Apple <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
---
M be/src/util/bit-util-test.cc
M be/src/util/sse-util.h
2 files changed, 38 insertions(+), 11 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10243
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9ec51bdfcb9455c93ff69827929a59fcccb81b80
Gerrit-Change-Number: 10243
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Jim Apple <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
