You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2016/06/02 21:22:29 UTC
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3291
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Allow compiler to use SSE3 and SSSE3 instructions.
We already depend on SSSE3 explicitly at runtime in
CpuInfo::VerifyCpuRequirements; this change just allows the compiler
(or Intel Intrinsic users) to explicitly depend upon that fact.
Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
---
M be/CMakeLists.txt
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/91/3291/1
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
> Compiling with SSE enabled, I don't feel confident that we'll
> actually get to the CpuInfo::VerifyCpuRequirements() check before
> executing an SSE3 instruction. Is there some way we can test this
> or compile the appropriate functions/module without enabling SSE3.
I doubt that we can manually prevent this instruction from showing up before we check for it. Global variable constructors are permitted to run before main() starts.
We can ask GCC to run the check as soon as possible, but other functions may make the same request and be competing:
https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Attributes.html#C_002b_002b-Attributes
It would be hard to compile certain modules without -msse3 and -mssse3, not only due to the cmake surgery, but also because we don't know which modules will have globals that could run before the check.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 2:
> > Aren't these already on by default? I'm pretty sure impalad ends
> > up with SSSE3 instructions. What's the default -march for our
> > toolchain gcc?
>
> I checked by using an SSE3 Intel Intrisic without adding this
> compiler option. It caused the build to fail with "target specific
> option mismatch"
SSE3 shouldn't be enabled but SSSE3 might be. But it might not be.
However, what's the motivation for this change? Do you see a tangible benefit? We could do it, but there might be some Impala users out there running on AMD Bulldozer (2011) even though we technically require SSSE3.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has abandoned this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Abandoned
Too much bookkeepping necessary to deal with older machines, ec2 machines, etc.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3291/1/be/CMakeLists.txt
File be/CMakeLists.txt:
Line 29: # -msse3, -mssse3: Enable sse3 and ssse3 compiler intrinsics
This is misleading, it enables the intrinsics but also allows the compiler to emit the instructions elsewhere.
Maybe "Allow compiler to emit sse3 and ssse3 instructions."
I'm not sure if this also implies MMX, SSE and SSE2.
An alternative would be to set -march=core2, since that includes the minimum feature-set we'd expect.
"Intel Core 2 CPU with 64-bit extensions, MMX, SSE, SSE2, SSE3 and SSSE3 instruction set support."
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#2).
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Allow compiler to use SSE3 and SSSE3 instructions.
We already depend on SSSE3 explicitly at runtime in
CpuInfo::VerifyCpuRequirements; this change just allows the compiler
(or Intel Intrinsic users) to explicitly depend upon that fact.
Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
---
M be/CMakeLists.txt
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/91/3291/2
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 2:
> > > Aren't these already on by default? I'm pretty sure impalad
> ends
> > > up with SSSE3 instructions. What's the default -march for our
> > > toolchain gcc?
> >
> > I checked by using an SSE3 Intel Intrisic without adding this
> > compiler option. It caused the build to fail with "target
> specific
> > option mismatch"
>
> SSE3 shouldn't be enabled but SSSE3 might be. But it might not be.
>
> However, what's the motivation for this change? Do you see a
> tangible benefit? We could do it, but there might be some Impala
> users out there running on AMD Bulldozer (2011) even though we
> technically require SSSE3.
The purpose is to make byte shuffles and unaligned 16-byte SIMD loads available for use with some of the SIMD optimizations that are being done by the Intel contributors.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
> Aren't these already on by default? I'm pretty sure impalad ends
> up with SSSE3 instructions. What's the default -march for our
> toolchain gcc?
I checked by using an SSE3 Intel Intrisic without adding this compiler option. It caused the build to fail with "target specific option mismatch"
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
Aren't these already on by default? I'm pretty sure impalad ends up with SSSE3 instructions. What's the default -march for our toolchain gcc?
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 2:
> > > > Aren't these already on by default? I'm pretty sure impalad
> > ends
> > > > up with SSSE3 instructions. What's the default -march for
> our
> > > > toolchain gcc?
> > >
> > > I checked by using an SSE3 Intel Intrisic without adding this
> > > compiler option. It caused the build to fail with "target
> > specific
> > > option mismatch"
> >
> > SSE3 shouldn't be enabled but SSSE3 might be. But it might not
> be.
> >
> > However, what's the motivation for this change? Do you see a
> > tangible benefit? We could do it, but there might be some Impala
> > users out there running on AMD Bulldozer (2011) even though we
> > technically require SSSE3.
>
> The purpose is to make byte shuffles and unaligned 16-byte SIMD
> loads available for use with some of the SIMD optimizations that
> are being done by the Intel contributors.
I see. See IMPALA-1646 and IMPALA-1399 for some examples of CPUs that Impala would no longer run after making this change. While we never went back and claimed support for pre-SSSE3, it probably does work today. Could you check with Justin to make sure he's okay with really no longer working on these CPUs?
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
Compiling with SSE enabled, I don't feel confident that we'll actually get to the CpuInfo::VerifyCpuRequirements() check before executing an SSE3 instruction. Is there some way we can test this or compile the appropriate functions/module without enabling SSE3.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
Good points. My inclination is that it's not worth the trouble to try to fail gracefully then, since it's unlikely that anyone will be trying to run Impala on hardware that old.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) Allow compiler to use SSE3 and SSSE3 instructions.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Allow compiler to use SSE3 and SSSE3 instructions.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3291/1/be/CMakeLists.txt
File be/CMakeLists.txt:
Line 29: # -msse3, -mssse3: Enable sse3 and ssse3 compiler intrinsics
> This is misleading, it enables the intrinsics but also allows the compiler
Our standard arch setting enables up to SSE2:
$ g++ -march=x86-64 -dM -E - < /dev/null | egrep "MMX|SSE" | sort
#define __MMX__ 1
#define __SSE__ 1
#define __SSE2__ 1
#define __SSE2_MATH__ 1
#define __SSE_MATH__ 1
I changed the comment as you requested.
--
To view, visit http://gerrit.cloudera.org:8080/3291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib045e00b5aa71cc6ef16dbda160bfdc7b765158a
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes