Fuse icmp/fcmp with select

Fuse icmp/fcmp with select

Allows the optimization of pairs of operations, including 64-bit compares and
selects as well as preparing for idioms (minsd, maxsd, etc.).

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-12-04 04:44:50]

Description was changed from

==========
Fuse icmp/fcmp with select

Allows the optimization of pairs of operations, including 64-bit compares and
selects as well as preparing for idioms (minsd, maxsd, etc.).

BUG=
==========

to

==========
Fuse icmp/fcmp with select

Allows the optimization of pairs of operations, including 64-bit compares and
selects as well as preparing for idioms (minsd, maxsd, etc.).

BUG=
==========

[sehr, 2015-12-04 04:44:50]

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

[sehr, 2015-12-04 04:45:12]

It's a big one, and in preparation for some peepholes.  PTAL.

[sehr, 2015-12-05 16:51:22]

On 2015/12/04 04:45:12, sehr (please use this account) wrote:
> It's a big one, and in preparation for some peepholes.  PTAL.

And I discovered the unittests stopped working -- PLEASE WAIT TO REVIEW.

[sehr, 2015-12-05 23:58:21]

On 2015/12/05 16:51:22, sehr (please use this account) wrote:
> On 2015/12/04 04:45:12, sehr (please use this account) wrote:
> > It's a big one, and in preparation for some peepholes.  PTAL.
> 
> And I discovered the unittests stopped working -- PLEASE WAIT TO REVIEW.

OK.  Unittests work after some consultations with Jan.  PTAL.

[John, 2015-12-07 13:07:44]

I'll leave the final approval to Jim.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:778: /// variable of the operation if Br ==
nullptr.
The comment needs to be updated.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:236: auto ConsumerKind =
getConsumerKind(&Instr);
I personally like using auto **everywhere** but unfortunately, we don't doo this
in subzero.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2507: if (Consumer != nullptr) {
Consider moving this block down to where InstSelect are handled below.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2658: 
assert(T != nullptr), maybe?

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2716: if (typeElementType(Ty) == IceType_i1)
{
optional: maybe create a helper function for converting between i1 -> iN
vectors.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4806: void
TargetX86Base<Machine>::lowerMove(Variable *Dest, Operand *Src,
What do you think about asserting !Src->isRematerializable (and, possibly
!Dest->isRematerializable) here?

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4817: if (IsRedefinition) {
I would personally do

_mov(DestLo, T_Lo);
if (IsRedefinition) {
  _set_dest_redefined();
}

but this is (1) my personal task; and (2) not idiomatic in the x86 backend.

[Jim Stichnoth, 2015-12-08 18:56:00]

https://codereview.chromium.org/1497033002/diff/120001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1112: char buf[30];
This style of generating the opcode string bothers me for a few reasons:

1. It's similar to above but doesn't use TypeAttributes[].

2. It ignores the class's Opcode static field (where the "andnp" could
conceivably come from).

3. The string could just be something like  (this->getDest()->getType() ==
IceType_f64) ? "andnpd" : "andnps"  without all the buf[] machinery.

I would probably improve this, and existing emit() implementations, by changing
the sdss column of ICETYPEX8632_TABLE and ICETYPEX8664_TABLE to be just "s" and
"d", then change the various FP scalar Opcode strings to append the "s", then
use the Opcode plus the TypeAttributes[] to form the emit string.

If that doesn't make sense, I can mock up the change to demonstrate.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:236: auto ConsumerKind =
getConsumerKind(&Instr);
On 2015/12/07 13:07:44, John wrote:
> I personally like using auto **everywhere** but unfortunately, we don't doo
this
> in subzero.

I don't think auto should be used here.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2560: }
May be an opportunity to put an early return in each block, and turn "else if"
into just "if".

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2720: llvm_unreachable("unexpected type");
I think report_fatal_error would be better here.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4817: if (IsRedefinition) {
On 2015/12/07 13:07:44, John wrote:
> I would personally do
> 
> _mov(DestLo, T_Lo);
> if (IsRedefinition) {
>   _set_dest_redefined();
> }
> 
> but this is (1) my personal task; and (2) not idiomatic in the x86 backend.

I agree with John's preference on this.

Thinking ahead, I might prefer something like:

  _redefined(_mov(DestLo, T_Lo), IsRedefinition);

but this would require _mov() and friends to return some non-void result (Inst*?
Variable*?).  That function might look something like:

  void _redefined(Inst *, bool IsRedefinition = true);

(Or it could return its Inst* argument instead of being void.)

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5723: // TODO(stichnot): Use xorps/xorpd
instead of pxor.
TODONE?

[sehr, 2015-12-15 20:45:44]

I addressed most of the comments (I hope), but kicked two issues down the road:

1) I didn't implement _redefined for ARM, because of the divergent assertions in
_mov and _mov_redefined there.
2) I didn't refactor the SSE[2] instructions as I think I understand how to do.

PTAL.

https://codereview.chromium.org/1497033002/diff/120001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1112: char buf[30];
On 2015/12/08 18:55:59, stichnot wrote:
> This style of generating the opcode string bothers me for a few reasons:
> 
> 1. It's similar to above but doesn't use TypeAttributes[].
> 
> 2. It ignores the class's Opcode static field (where the "andnp" could
> conceivably come from).
> 
> 3. The string could just be something like  (this->getDest()->getType() ==
> IceType_f64) ? "andnpd" : "andnps"  without all the buf[] machinery.
> 
> I would probably improve this, and existing emit() implementations, by
changing
> the sdss column of ICETYPEX8632_TABLE and ICETYPEX8664_TABLE to be just "s"
and
> "d", then change the various FP scalar Opcode strings to append the "s", then
> use the Opcode plus the TypeAttributes[] to form the emit string.
> 
> If that doesn't make sense, I can mock up the change to demonstrate.

OK, I think there's a larger commonality to the SSE[2] opcodes here, and we need
a common base class for them.  I  have an idea how to address this more
generally, but would like to consult with you and John before undertaking it. 
As this CL is already getting way out of control, I've fallen back to addressing
your (1).  PTAL.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:778: /// variable of the operation if Br ==
nullptr.
On 2015/12/07 13:07:44, John wrote:
> The comment needs to be updated.

Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:236: auto ConsumerKind =
getConsumerKind(&Instr);
On 2015/12/07 13:07:44, John wrote:
> I personally like using auto **everywhere** but unfortunately, we don't doo
this
> in subzero.

Jim shot you down :-).

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:236: auto ConsumerKind =
getConsumerKind(&Instr);
On 2015/12/08 18:55:59, stichnot wrote:
> On 2015/12/07 13:07:44, John wrote:
> > I personally like using auto **everywhere** but unfortunately, we don't doo
> this
> > in subzero.
> 
> I don't think auto should be used here.

Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2507: if (Consumer != nullptr) {
On 2015/12/07 13:07:44, John wrote:
> Consider moving this block down to where InstSelect are handled below.

It's up here as a short-circuit before any lowering happens.  In particular, I
envision generating maxss and friends, where we don't do any ucomiss instruction
at all.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2560: }
On 2015/12/08 18:55:59, stichnot wrote:
> May be an opportunity to put an early return in each block, and turn "else if"
> into just "if".

Done, here and everywhere.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2658: 
On 2015/12/07 13:07:44, John wrote:
> assert(T != nullptr), maybe?

Agreed.  Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2716: if (typeElementType(Ty) == IceType_i1)
{
On 2015/12/07 13:07:44, John wrote:
> optional: maybe create a helper function for converting between i1 -> iN
> vectors.

I think there's only one use (for now), but intend to revisit integer vector
operations considerably.  Punting for now.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2720: llvm_unreachable("unexpected type");
On 2015/12/08 18:55:59, stichnot wrote:
> I think report_fatal_error would be better here.

Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4806: void
TargetX86Base<Machine>::lowerMove(Variable *Dest, Operand *Src,
On 2015/12/07 13:07:44, John wrote:
> What do you think about asserting !Src->isRematerializable (and, possibly
> !Dest->isRematerializable) here?

Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4817: if (IsRedefinition) {
On 2015/12/08 18:55:59, stichnot wrote:
> On 2015/12/07 13:07:44, John wrote:
> > I would personally do
> > 
> > _mov(DestLo, T_Lo);
> > if (IsRedefinition) {
> >   _set_dest_redefined();
> > }
> > 
> > but this is (1) my personal task; and (2) not idiomatic in the x86 backend.
> 
> I agree with John's preference on this.
> 
> Thinking ahead, I might prefer something like:
> 
>   _redefined(_mov(DestLo, T_Lo), IsRedefinition);
> 
> but this would require _mov() and friends to return some non-void result
(Inst*?
> Variable*?).  That function might look something like:
> 
>   void _redefined(Inst *, bool IsRedefinition = true);
> 
> (Or it could return its Inst* argument instead of being void.)

Done.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4817: if (IsRedefinition) {
On 2015/12/07 13:07:44, John wrote:
> I would personally do
> 
> _mov(DestLo, T_Lo);
> if (IsRedefinition) {
>   _set_dest_redefined();
> }
> 
> but this is (1) my personal task; and (2) not idiomatic in the x86 backend.

Followed Jim's extension of this suggestion.

https://codereview.chromium.org/1497033002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5723: // TODO(stichnot): Use xorps/xorpd
instead of pxor.
On 2015/12/08 18:55:59, stichnot wrote:
> TODONE?

TODONEDONE.

[Jim Stichnoth, 2015-12-16 01:00:37]

lgtm

https://codereview.chromium.org/1497033002/diff/140001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/140001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1114: buf, llvm::array_lengthof(buf), "andn%s",
Instead of:
  "andn%s",
Can you use:
  "%s%s", Opcode,

i.e. reduce the duplication of the "andn" string.

If that works, maybe all these emit() functions can be refactored and reduced by
a lot.

https://codereview.chromium.org/1497033002/diff/140001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (left):

https://codereview.chromium.org/1497033002/diff/140001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:480: void _mov_redefined(Variable *Dest, Operand
*Src0) {
It's gone - nice!

[sehr, 2015-12-16 01:34:04]

Thanks for your patience.  Waiting for another test run to commit.

https://codereview.chromium.org/1497033002/diff/140001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1497033002/diff/140001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1114: buf, llvm::array_lengthof(buf), "andn%s",
On 2015/12/16 01:00:37, stichnot wrote:
> Instead of:
>   "andn%s",
> Can you use:
>   "%s%s", Opcode,
> 
> i.e. reduce the duplication of the "andn" string.
> 
> If that works, maybe all these emit() functions can be refactored and reduced
by
> a lot.

Done.

https://codereview.chromium.org/1497033002/diff/140001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (left):

https://codereview.chromium.org/1497033002/diff/140001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:480: void _mov_redefined(Variable *Dest, Operand
*Src0) {
On 2015/12/16 01:00:37, stichnot wrote:
> It's gone - nice!

Yep, except for ARM -- need Joao's help for that.

[sehr, 2015-12-16 01:34:58]

Description was changed from

==========
Fuse icmp/fcmp with select

Allows the optimization of pairs of operations, including 64-bit compares and
selects as well as preparing for idioms (minsd, maxsd, etc.).

BUG=
==========

to

==========
Fuse icmp/fcmp with select

Allows the optimization of pairs of operations, including 64-bit compares and
selects as well as preparing for idioms (minsd, maxsd, etc.).

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-12-16 01:34:59]

Committed patchset #8 (id:140001) manually as
e398428c3ef8ed2339c0f334dd30a7d3f0ed4b6e (presubmit successful).