ARM32 Vector lowering - scalarize select

ARM32 Vector lowering - scalarize select

With this change, we pass the select crosstest.

Since this would have introduced a three-argument version of
scalarizeInstruction, I decided to generalize it using templates.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[Eric Holk, 2016-02-10 21:21:20]

eholk@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, sehr@chromium.org,
stichnot@chromium.org

[Eric Holk, 2016-02-10 22:00:16]

Description was changed from

==========
ARM32 Vector lowering - scalarize select

With this change, we can pass the select crosstest using the
non-integrated assembler.

Since this would have introduced a three-argument version of
scalarizeInstruction, I decided to generalize it using templates.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
==========

to

==========
ARM32 Vector lowering - scalarize select

With this change, we pass the select crosstest.

Since this would have introduced a three-argument version of
scalarizeInstruction, I decided to generalize it using templates.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
==========

[John, 2016-02-11 15:02:41]

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (left):

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:476: F &&MakeInstruction) {
I didn't see this before, but I am not a fan of this F &&MakeFunction. Nothing
wrong with lambdas, and unfortunately this is the most efficient way to pass a
lambda, but you are giving up the explicit signature. You are also instantiating
this template with different arguments for each call site. Consider changing
this argument to an std::function and, if this becomes a bottleneck, we can
revisit and change to a dependent type.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:473: /// (Variable *Dest, Variable *Src0, Variable
*Src1) -> Instr *.
out of date comment...

that's why I would encourage having an non-dependent type for F. would something
like:

template <typename... Operand>
void scalarizeInstruction(Variable *Dest, std::function<Instr *(Variable *,
Operand *...)> MakeInstruction, Operand *... Src)

work?

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:475: void scalarizeInstruction(Variable *Dest, F
&&MakeInstruction,
MakeInstruction is a parameter, hence it is named in CamelCaseStyle. I would
like to suggest, though, that we change it to abide by the functionNamingStyle.
Thoughts?

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:488: auto MakeExtract = [&](Operand *Src) {
please, no default capture -- capture what you need, even if it means capturing
every local plus this.

Also, I know MakeExtract is local, but only capture by reference if you need to
change the local -- otherwise capture by value. Given that everything you need
to capture is a pointer already, capturing by ref is strictly worse.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:492: Variable *Op = Func->makeVariable(ElementTy);
why did you use makeVariable instead of makeReg?

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:492: Variable *Op = Func->makeVariable(ElementTy);
auto here?

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:499: auto *Arith = MakeInstruction(Res,
MakeExtract(Srcs)...);
I just noticed that MakeInstruction is misleading. I was reading the code as if
MakeInstruction would only create the instructions -- but it inserts as well.
Consider renaming it to InsertScalarInstruction.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:500: genTargetHelperCallFor(Arith);
Calling this method here is bad -- really bad.

This method is supposed to be called before lowering starts to pre-generate
calls so that the frame size calculation works.

You probably need to generate the Insert/Op/Extract operations before the
TargetLowering::genTargetHelperCallFors() is invoked.

[Jim Stichnoth, 2016-02-11 15:26:07]

https://codereview.chromium.org/1683243003/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1683243003/diff/60001/Makefile.standalone#new...
Makefile.standalone:452: -i arm32,neon,test_select
Nice!

You should be able to just remove this line since it is covered by "-i
arm32,neon" above.

Also, note that many ARM vector tests are currently disabled via "#ifndef ARM32"
in the crosstest driver files.  It means that the tests themselves get
translated, perhaps with udf trap instructions where the integrated assembler is
not yet ready, just that they are not actually invoked.  Ultimately you'll need
to remove those #ifdefs as well.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:488: auto MakeExtract = [&](Operand *Src) {
Check whether the experts (hi John!) are OK with this use of default capture.

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/select-vec.ll:12: 
I think these tests generally only have one blank line here.

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/select-vec.ll:14: define internal <4 x i32>
@select4i32(<4 x i32> %a, <4 x i32> %b, <4 x i1> %s) {
You should probably include tests on all the vector types.

Also, any reasons the function args are listed in a different order from the
select instruction?

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/select-vec.ll:51: 
remove trailing blank line

[Eric Holk, 2016-02-11 19:17:23]

https://codereview.chromium.org/1683243003/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1683243003/diff/60001/Makefile.standalone#new...
Makefile.standalone:452: -i arm32,neon,test_select
On 2016/02/11 15:26:07, stichnot wrote:
> Nice!
> 
> You should be able to just remove this line since it is covered by "-i
> arm32,neon" above.
> 
> Also, note that many ARM vector tests are currently disabled via "#ifndef
ARM32"
> in the crosstest driver files.  It means that the tests themselves get
> translated, perhaps with udf trap instructions where the integrated assembler
is
> not yet ready, just that they are not actually invoked.  Ultimately you'll
need
> to remove those #ifdefs as well.

Ok. I removed this line, and I'll go looking for the #ifdefs soon.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:475: void scalarizeInstruction(Variable *Dest, F
&&MakeInstruction,
On 2016/02/11 15:02:41, John wrote:
> MakeInstruction is a parameter, hence it is named in CamelCaseStyle. I would
> like to suggest, though, that we change it to abide by the
functionNamingStyle.
> Thoughts?

That sounds alright to me. I went ahead and made the change.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:488: auto MakeExtract = [&](Operand *Src) {
On 2016/02/11 15:02:41, John wrote:
> please, no default capture -- capture what you need, even if it means
capturing
> every local plus this.
> 
> Also, I know MakeExtract is local, but only capture by reference if you need
to
> change the local -- otherwise capture by value. Given that everything you need
> to capture is a pointer already, capturing by ref is strictly worse.

Done.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:492: Variable *Op = Func->makeVariable(ElementTy);
On 2016/02/11 15:02:41, John wrote:
> auto here?

Done.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:492: Variable *Op = Func->makeVariable(ElementTy);
On 2016/02/11 15:02:41, John wrote:
> why did you use makeVariable instead of makeReg?

makeReg is defined in TargetARM32, and this is in the target-independent code.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:499: auto *Arith = MakeInstruction(Res,
MakeExtract(Srcs)...);
On 2016/02/11 15:02:41, John wrote:
> I just noticed that MakeInstruction is misleading. I was reading the code as
if
> MakeInstruction would only create the instructions -- but it inserts as well.
> Consider renaming it to InsertScalarInstruction.

Done.

https://codereview.chromium.org/1683243003/diff/60001/src/IceTargetLowering.h...
src/IceTargetLowering.h:500: genTargetHelperCallFor(Arith);
On 2016/02/11 15:02:41, John wrote:
> Calling this method here is bad -- really bad.
> 
> This method is supposed to be called before lowering starts to pre-generate
> calls so that the frame size calculation works.
> 
> You probably need to generate the Insert/Op/Extract operations before the
> TargetLowering::genTargetHelperCallFors() is invoked.

Scalarize is called by genTargetHelperCallsFor, so it's not so bad here. This is
also how it worked back when scalarization only happened for x86.

I added a flag to make sure we only call this during the correct phase.

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/select-vec.ll:12: 
On 2016/02/11 15:26:07, stichnot wrote:
> I think these tests generally only have one blank line here.

Done.

https://codereview.chromium.org/1683243003/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/select-vec.ll:14: define internal <4 x i32>
@select4i32(<4 x i32> %a, <4 x i32> %b, <4 x i1> %s) {
On 2016/02/11 15:26:07, stichnot wrote:
> You should probably include tests on all the vector types.

Okay. How much coverage do we have in the crosstests? I've been using shorter
tests lately because I expect the crosstests to do a better job testing these
instruction soon anyway.

> 
> Also, any reasons the function args are listed in a different order from the
> select instruction?

Not really. I can change it.

[Jim Stichnoth, 2016-02-12 02:32:50]

https://codereview.chromium.org/1683243003/diff/80001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1683243003/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:311: GeneratingTargetHelpers = true;
It would be nice to use a RAII helper object for this.

Maybe BoolFlagSaver could be lifted out of the x86 target and used here.

I don't know whether I would put it in IceDefs.h, IceUtils.h,
IceTargetLowering.h, or what.

[Eric Holk, 2016-02-12 18:38:15]

I realized the way I was generating the extract instructions led to
indeterminate evaluation order, and therefore the final output was
unpredictable. I fixed this by specializing the three versions of
scalarizeInstruction that we are actually using.

https://codereview.chromium.org/1683243003/diff/80001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1683243003/diff/80001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:311: GeneratingTargetHelpers = true;
On 2016/02/12 02:32:50, stichnot wrote:
> It would be nice to use a RAII helper object for this.
> 
> Maybe BoolFlagSaver could be lifted out of the x86 target and used here.
> 
> I don't know whether I would put it in IceDefs.h, IceUtils.h,
> IceTargetLowering.h, or what.

Done.

[Jim Stichnoth, 2016-02-12 22:41:31]

https://codereview.chromium.org/1683243003/diff/120001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1683243003/diff/120001/src/IceUtils.h#newcode126
src/IceUtils.h:126: /// A helper class to ease the settings of
RandomizationPoolingPause to disable
This comment is now completely wrong. :)

https://codereview.chromium.org/1683243003/diff/120001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/120001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/select-vec.ll:264: 
don't forget to remove trailing blank line

[Eric Holk, 2016-02-12 23:17:27]

This fixes issues Jim raised on the last CL, and also introduces a new and
improved way of avoiding nondeterministic argument evaluation.

https://codereview.chromium.org/1683243003/diff/120001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/120001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/select-vec.ll:264: 
On 2016/02/12 22:41:31, stichnot wrote:
> don't forget to remove trailing blank line

Oops, I thought I made that change but it must have gotten lost on a branch
somewhere.

[Jim Stichnoth, 2016-02-13 05:22:09]

I really don't know what kind of template wizardry you have conjured up, so
science dog says lgtm.

https://codereview.chromium.org/1683243003/diff/140001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1683243003/diff/140001/src/IceUtils.h#newcode126
src/IceUtils.h:126: /// A helper class to ensure that a boolean flag is restored
to its previous
Could you add "RAII" to the comment somewhere?  That should help people (like
me) find it more easily in the future.

https://codereview.chromium.org/1683243003/diff/140001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/140001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/select-vec.ll:3: ; TODO(eholk): This test will need to
be updated once comparison is no
Can this comment be reflowed to 80-col?

[Eric Holk, 2016-02-16 23:05:30]

During presubmit checks, I found out g++ had a bug that made it not work. The
latest updates refactor the code slightly to work around the bug.

Before, the code looked something like this:

auto fn = [](Src) { /* body */ }
callFn([](){ fn(Srcs) }...)

where Srcs is a parameter pack.

g++ 4.8 doesn't expand parameter packs in lambdas, so now the code works like
this:

auto fnThunk = [](Src) { return []() { /* body */ }; };
callFn(fnThunk(Srcs)...)

This has equivalent behavior, but only does parameter pack expansion inside a
function call.

https://codereview.chromium.org/1683243003/diff/140001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1683243003/diff/140001/src/IceUtils.h#newcode126
src/IceUtils.h:126: /// A helper class to ensure that a boolean flag is restored
to its previous
On 2016/02/13 05:22:09, stichnot wrote:
> Could you add "RAII" to the comment somewhere?  That should help people (like
> me) find it more easily in the future.

Sure.

https://codereview.chromium.org/1683243003/diff/140001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/select-vec.ll (right):

https://codereview.chromium.org/1683243003/diff/140001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/select-vec.ll:3: ; TODO(eholk): This test will need to
be updated once comparison is no
On 2016/02/13 05:22:09, stichnot wrote:
> Can this comment be reflowed to 80-col?

Done.

[Jim Stichnoth, 2016-02-17 04:01:56]

still lgtm

https://codereview.chromium.org/1683243003/diff/180001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1683243003/diff/180001/src/IceUtils.h#newcode127
src/IceUtils.h:127: /// value upon function exist.
exit

[Eric Holk, 2016-02-17 19:09:50]

Description was changed from

==========
ARM32 Vector lowering - scalarize select

With this change, we pass the select crosstest.

Since this would have introduced a three-argument version of
scalarizeInstruction, I decided to generalize it using templates.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
==========

to

==========
ARM32 Vector lowering - scalarize select

With this change, we pass the select crosstest.

Since this would have introduced a three-argument version of
scalarizeInstruction, I decided to generalize it using templates.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[Eric Holk, 2016-02-17 19:09:51]

Committed patchset #11 (id:200001) manually as
d6cf6b3890ad64a92c9688d35ca7dd29dc7255ac (presubmit successful).