Subzero: Add support for SSE4.1 instructions.

Subzero: Add support for SSE4.1 instructions.

* Add initial support for code generation with SSE4.1 instructions. The
following operations are affected:
 - multiplication with v4i32
 - select
 - insertelement
 - extractelement

* Add appropriate lit checks for SSE4.1 instructions. Run the crosstests
in both SSE2 and SSE4.1 mode.

* Introduce the -mattr flag to llvm2ice to control which instruction set
gets used.

BUG=none
R=jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=0a45051

[Jim Stichnoth, 2014-07-29 23:01:17]

Otherwise lgtm.

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:539: assert(getDest()->getType() == IceType_v4i32 ||
It would be nice if the asserts here and in lowerArithmetic() were syntactically
basically the same.

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:1175: assert(getSrc(0)->getType() == IceType_v8i16 ||
Same comment about syntactically equivalent asserts.

https://codereview.chromium.org/427843002/diff/50011/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:444: X86InstructionSet InstructionSet;
Make this const?

https://codereview.chromium.org/427843002/diff/50011/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arith.ll (right):

https://codereview.chromium.org/427843002/diff/50011/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arith.ll:322: define <4 x i32>
@test_shl_v4i32(<4 x i32> %arg0, <4 x i32> %arg1) {
Probably want to add "SSE41-LABEL: test_shl_v4i32" to make sure the above pmulld
appears in the right function.

[wala, 2014-07-30 00:11:24]

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:539: assert(getDest()->getType() == IceType_v4i32 ||
On 2014/07/29 23:01:16, stichnot wrote:
> It would be nice if the asserts here and in lowerArithmetic() were
syntactically
> basically the same.

I'm not sure what you mean by this. I don't see any relevant asserts in
lowerArithmetic(). Are you suggesting that the asserts should syntactically
match the conditions that generate the instructions in lowerArithmetic()  /
lowerExtractElement() (the latter for pextrw and pextrd)? I have at least two
issues with that suggestion:

1) Breaking up the conditions into multiple assert statements allows for more
precise error messsages.

2) The instructions may be used in other contexts that don't fit the conditions
present in lowerArithmetic() / lowerExtractElement(). For instance, there is no
requirement preventing pextrw or pextrd from having Src0 as a v4f32, despite the
fact that the condition in lowerExtractElement() checks that Src0 is not a
v4f32.

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:1175: assert(getSrc(0)->getType() == IceType_v8i16 ||
On 2014/07/29 23:01:16, stichnot wrote:
> Same comment about syntactically equivalent asserts.

See reply above.

https://codereview.chromium.org/427843002/diff/50011/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:444: X86InstructionSet InstructionSet;
On 2014/07/29 23:01:17, stichnot wrote:
> Make this const?

Done.

https://codereview.chromium.org/427843002/diff/50011/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arith.ll (right):

https://codereview.chromium.org/427843002/diff/50011/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arith.ll:322: define <4 x i32>
@test_shl_v4i32(<4 x i32> %arg0, <4 x i32> %arg1) {
On 2014/07/29 23:01:17, stichnot wrote:
> Probably want to add "SSE41-LABEL: test_shl_v4i32" to make sure the above
pmulld
> appears in the right function.

Done.

[wala, 2014-07-30 00:13:17]

On 2014/07/30 00:11:24, wala wrote:
> https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp
> File src/IceInstX8632.cpp (right):
> 
>
https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
> src/IceInstX8632.cpp:539: assert(getDest()->getType() == IceType_v4i32 ||
> On 2014/07/29 23:01:16, stichnot wrote:
> > It would be nice if the asserts here and in lowerArithmetic() were
> syntactically
> > basically the same.
> 
> I'm not sure what you mean by this. I don't see any relevant asserts in
> lowerArithmetic(). Are you suggesting that the asserts should syntactically
> match the conditions that generate the instructions in lowerArithmetic()  /
> lowerExtractElement() (the latter for pextrw and pextrd)? I have at least two
> issues with that suggestion:
> 
> 1) Breaking up the conditions into multiple assert statements allows for more
> precise error messsages.
> 
> 2) The instructions may be used in other contexts that don't fit the
conditions
> present in lowerArithmetic() / lowerExtractElement(). For instance, there is
no
> requirement preventing pextrw or pextrd from having Src0 as a v4f32, despite
the
> fact that the condition in lowerExtractElement() checks that Src0 is not a
> v4f32.
> 

I meant to say pextrb along with pextrw and pextrd, of course.

[Jim Stichnoth, 2014-07-30 02:58:56]

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:539: assert(getDest()->getType() == IceType_v4i32 ||
On 2014/07/30 00:11:24, wala wrote:
> On 2014/07/29 23:01:16, stichnot wrote:
> > It would be nice if the asserts here and in lowerArithmetic() were
> syntactically
> > basically the same.
> 
> I'm not sure what you mean by this. I don't see any relevant asserts in
> lowerArithmetic(). 

I'm talking about lines 1244-1245 of IceTargetLoweringX8632.cpp, which are part
of lowerArithmetic():

  if (Dest->getType() == IceType_v8i16 ||
      (InstructionSet >= SSE4_1 && Dest->getType() == IceType_v4i32)) {

compared to the (slightly paraphrased) asserts here:

  assert(getDest()->getType() == IceType_v4i32 ||
         getDest()->getType() == IceType_v8i16);
  assert(getDest()->getType() != IceType_v4i32 ||
         InstructionSet >= SSE4_1);

> Are you suggesting that the asserts should syntactically
> match the conditions that generate the instructions in lowerArithmetic()  /
> lowerExtractElement() (the latter for pextrw and pextrd)?

Basically, yes (for the conditions I transcribed above).  I had to do more
head-scratching than should be necessary to convince myself that the two
conditions are the same.  What if both sections had code like this:

  bool Blah1 = (getDest()->getType() == IceType_v4i32 ||
                getDest()->getType() == IceType_v8i16);
  bool Blah2 = (getDest()->getType() != IceType_v4i32 ||
                InstructionSet >= SSE4_1);

and the first condition were rewritten as:

  if (Blah1 && Blah2) {

and the asserts were rewritten as:

  assert(Blah1);
  assert(Blah2);

> I have at least two
> issues with that suggestion:
> 
> 1) Breaking up the conditions into multiple assert statements allows for more
> precise error messsages.

My suggestion allows for that.

> 
> 2) The instructions may be used in other contexts that don't fit the
conditions
> present in lowerArithmetic() / lowerExtractElement(). For instance, there is
no
> requirement preventing pextrw or pextrd from having Src0 as a v4f32, despite
the
> fact that the condition in lowerExtractElement() checks that Src0 is not a
> v4f32.

OK, this is a great point.  I would adjust my suggestion as follows.  There is
some set of basic structural conditions on the instruction, which the emit()
routine asserts.  The lowering code uses that same set of conditions, plus
possibly some more restrictive conditions, to determine whether to lower to that
instruction.

[jvoung (off chromium), 2014-07-30 04:30:11]

lgtm

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh#ne...
crosstest/runtests.sh:16: for attribute in ${ATTRIBUTES} ; do
We might want to start parallelizing some of these compiles, once we add more
attributes =)

Maybe I'll look into that, if you guys haven't already started looking into it.

https://codereview.chromium.org/427843002/diff/70001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/70001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:130: "mattr", cl::desc("X86 target attributes"),
This is okay. Just a note though, that the notation is a bit different from
LLVM's mattr, in case we expect to share a similar style of commandline flags.
LLVM uses "+feature" or "-feature" for toggling on and off features. That's
mostly useful when there isn't a strict ordering on the features.

https://codereview.chromium.org/427843002/diff/70001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-ops.ll (right):

https://codereview.chromium.org/427843002/diff/70001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-ops.ll:158: ; SSE41: pshufd
Is the plan to use extractps later?

[Jim Stichnoth, 2014-07-30 04:45:48]

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh#ne...
crosstest/runtests.sh:16: for attribute in ${ATTRIBUTES} ; do
On 2014/07/30 04:30:11, jvoung wrote:
> We might want to start parallelizing some of these compiles, once we add more
> attributes =)
> 
> Maybe I'll look into that, if you guys haven't already started looking into
it.

Right, I forgot to mention the same in my review. :)

By parallelizing, you mean adding them to the lit framework, right?  That's on
"the list", but I didn't have any immediate plans to do it yet.

[wala, 2014-07-30 18:11:55]

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:539: assert(getDest()->getType() == IceType_v4i32 ||
On 2014/07/30 02:58:56, stichnot wrote:
> On 2014/07/30 00:11:24, wala wrote:
> > On 2014/07/29 23:01:16, stichnot wrote:
> > > It would be nice if the asserts here and in lowerArithmetic() were
> > syntactically
> > > basically the same.
> > 
> > I'm not sure what you mean by this. I don't see any relevant asserts in
> > lowerArithmetic(). 
> 
> I'm talking about lines 1244-1245 of IceTargetLoweringX8632.cpp, which are
part
> of lowerArithmetic():
> 
>   if (Dest->getType() == IceType_v8i16 ||
>       (InstructionSet >= SSE4_1 && Dest->getType() == IceType_v4i32)) {
> 
> compared to the (slightly paraphrased) asserts here:
> 
>   assert(getDest()->getType() == IceType_v4i32 ||
>          getDest()->getType() == IceType_v8i16);
>   assert(getDest()->getType() != IceType_v4i32 ||
>          InstructionSet >= SSE4_1);
> 
> > Are you suggesting that the asserts should syntactically
> > match the conditions that generate the instructions in lowerArithmetic()  /
> > lowerExtractElement() (the latter for pextrw and pextrd)?
> 
> Basically, yes (for the conditions I transcribed above).  I had to do more
> head-scratching than should be necessary to convince myself that the two
> conditions are the same.  What if both sections had code like this:
> 
>   bool Blah1 = (getDest()->getType() == IceType_v4i32 ||
>                 getDest()->getType() == IceType_v8i16);
>   bool Blah2 = (getDest()->getType() != IceType_v4i32 ||
>                 InstructionSet >= SSE4_1);
> 
> and the first condition were rewritten as:
> 
>   if (Blah1 && Blah2) {
> 
> and the asserts were rewritten as:
> 
>   assert(Blah1);
>   assert(Blah2);
> 
> > I have at least two
> > issues with that suggestion:
> > 
> > 1) Breaking up the conditions into multiple assert statements allows for
more
> > precise error messsages.
> 
> My suggestion allows for that.
> 
> > 
> > 2) The instructions may be used in other contexts that don't fit the
> conditions
> > present in lowerArithmetic() / lowerExtractElement(). For instance, there is
> no
> > requirement preventing pextrw or pextrd from having Src0 as a v4f32, despite
> the
> > fact that the condition in lowerExtractElement() checks that Src0 is not a
> > v4f32.
> 
> OK, this is a great point.  I would adjust my suggestion as follows.  There is
> some set of basic structural conditions on the instruction, which the emit()
> routine asserts.  The lowering code uses that same set of conditions, plus
> possibly some more restrictive conditions, to determine whether to lower to
that
> instruction.

Done.

Thanks for the explanation.

https://codereview.chromium.org/427843002/diff/50011/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:1175: assert(getSrc(0)->getType() == IceType_v8i16 ||
On 2014/07/30 00:11:24, wala wrote:
> On 2014/07/29 23:01:16, stichnot wrote:
> > Same comment about syntactically equivalent asserts.
> 
> See reply above.

Done.

https://codereview.chromium.org/427843002/diff/70001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/427843002/diff/70001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:130: "mattr", cl::desc("X86 target attributes"),
On 2014/07/30 04:30:11, jvoung wrote:
> This is okay. Just a note though, that the notation is a bit different from
> LLVM's mattr, in case we expect to share a similar style of commandline flags.
> LLVM uses "+feature" or "-feature" for toggling on and off features. That's
> mostly useful when there isn't a strict ordering on the features.

Okay.
We may have to migrate to that once we have features that are not linearly
ordered.

https://codereview.chromium.org/427843002/diff/70001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-ops.ll (right):

https://codereview.chromium.org/427843002/diff/70001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-ops.ll:158: ; SSE41: pshufd
On 2014/07/30 04:30:11, jvoung wrote:
> Is the plan to use extractps later?

No. I now think we should not be using extractps to implement extractelement.

extractps takes reg32/mem32 as the dest operand, so to implement extractelement
of v4f32 with extractps would require an extra operation (like movd) after
extractps to copy the result back into an xmm register. This seems inferior to
pshufd. LLVM uses pshufd even with -mattr=sse4.1.

[wala, 2014-07-30 19:44:42]

Committed patchset #7 manually as r0a45051 (presubmit successful).

[jvoung (off chromium), 2014-07-31 23:38:17]

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/427843002/diff/70001/crosstest/runtests.sh#ne...
crosstest/runtests.sh:16: for attribute in ${ATTRIBUTES} ; do
On 2014/07/30 04:45:47, stichnot wrote:
> On 2014/07/30 04:30:11, jvoung wrote:
> > We might want to start parallelizing some of these compiles, once we add
more
> > attributes =)
> > 
> > Maybe I'll look into that, if you guys haven't already started looking into
> it.
> 
> Right, I forgot to mention the same in my review. :)
> 
> By parallelizing, you mean adding them to the lit framework, right?  That's on
> "the list", but I didn't have any immediate plans to do it yet.

Ah sorry for the delay in the reply and I didn't get it done before vacation,
though I volunteered...

Yes, we could try to fit it into the lit framework. Some things to watch out
for, but hadn't considered fully:

(*) How to be able to loop through attributes, arches, and optlevels in the lit
framework, without have to unroll the loops?  Currently, our .ll files have
multiple run lines, and we end up with some duplication. That's okay for the
.ll, since we often have different CHECK lines and check-prefixes. These cross
tests just check the exit status though, so it's more amenable to just looping.

Perhaps this can be handled, if each test case has a "RUN" line that invokes a
script, and the script can then loop through. Then at least there is some
parallelism between the test cases, but not the loops within the test cases.

(*) Naming the test cases / setting up "config.suffixes": I think the test cases
would have to use a suffix that is not ".cpp" or ".ll", since one test case
consists of multiple .cpp/.ll files. That's a bit different from the usual use
of lit, where each .ll and .cpp are individual test cases. Maybe it doesn't
matter and we just make the individual tests have .py suffixes.

Otherwise, I had considered a quick and dirty use of bash "&" plus "wait"...