-
Notifications
You must be signed in to change notification settings - Fork 193
PARQUET-889: Fix compilation when SSE is enabled #257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you -- we should add an SSE build to Travis CI so we catch these issues.
As another matter, the USE_SSE
flag is a bit of an eyesore -- we might consider "dual compilation" classes that can utilize SIMD instructions. So if we have SSE4 available on our processor, then the library can select the SSE-enabled version of DictionaryEncoder.
src/parquet/util/cpu-info.h
Outdated
DCHECK(initialized_); | ||
return (hardware_flags_ & flag) != 0; | ||
} | ||
static bool IsSupported(int64_t flag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one function that really needs to be inlined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you suggest we keep it in the header ?
Here are the solution I can think of in order of preference:
- manual if + throw (or abort)
- Simply remove the DCHECK
- Make the logging.h a public header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say to remove the DCHECK, @majetideepak @xhochy do you have an opinion? whether the CpuInfo
is initialized is something we can test elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Away with it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me, let me remove it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with removing it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix has been pushed, I guess one can merge it as soon as the CI finishes :)
9415293
to
f08867b
Compare
I'm keeping an eye on it. If you enable Travis CI for your fork then we can get faster builds (right now we're in the general ASAF queue, which can sometimes get congested) |
bit-util.h was missing a couple of include. cpu-util.h was including parquet/logging.h which is an internal header so the implementations of the static functions have been moved in the source file.
The build passed https://travis-ci.org/daedric/parquet-cpp/builds/204262490 -- the Linux failures are because of coveralls (we should make coveralls only run on the main repo maybe). Merging |
Thanks for your contribution! |
My pleasure :) |
bit-util.h was missing a couple of include. cpu-util.h was including
parquet/logging.h which is an internal header so the implementations of
the static functions have been moved in the source file.