Merge compares and branches

Merge compares and branches

Generalize folding of icmp instructions into br.  64-bit comparisons are
considered as candidates unless they feed a select.

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-10-16 00:25:09]

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

[sehr, 2015-10-16 00:26:37]

I think I might want to improve the lowering of 64-bit icmps that are folded
into branches.

Folding into other operations (e.g., selects) remains TBD.

[sehr, 2015-10-16 00:27:05]

On 2015/10/16 00:26:37, sehr (please use this account) wrote:
> I think I might want to improve the lowering of 64-bit icmps that are folded
> into branches.
> 
> Folding into other operations (e.g., selects) remains TBD.

FWIW, SPEC runs, but I haven't measured performance yet.

[Jim Stichnoth, 2015-10-16 13:58:11]

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
File src/IceTargetLoweringX86Base.h (right):

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86Base.h:714: // Emit the code for a combined compare and
branch, or sets the destination
Probably use /// style for consistency?

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86Base.h:716: void lowerIcmpAndBr(const InstIcmp *Icmp,
const InstBr *Br = nullptr);
You're always calling this with 2 explicit args, so remove the default
parameter?

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:204: (getConsumerKind(&Instr) != CK_Br && //
Icmp64 only folds in branches
80-col

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2657: lowerIcmpAndBr(Inst, nullptr);
Maybe use "constexpr InstBr *Br = nullptr;" for clarity?

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2668: assert((Br == nullptr) && "not ready to
fold vector compare/branch");
Make this a report_fatal_error() instead?

Also, I can't see that we would *ever* be doing branch optimizations on bool
vector operations, so rephrase the error not to get people's hopes up? :)

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2916: _br(Traits::TableIcmp64[Index].C3,
Br->getTargetTrue(), Br->getTargetFalse());
80-col

https://chromiumcodereview.appspot.com/1407143002/diff/1/tests_lit/llvm2ice_t...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (left):

https://chromiumcodereview.appspot.com/1407143002/diff/1/tests_lit/llvm2ice_t...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:1019: ; CHECK: je
Here and below, I think it would be better to replace this je with jne (or
whatever), instead of deleting the check.

[sehr, 2015-10-16 18:10:23]

Thanks for the speedy review. PTAL.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
File src/IceTargetLoweringX86Base.h (right):

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86Base.h:714: // Emit the code for a combined compare and
branch, or sets the destination
On 2015/10/16 13:58:11, stichnot wrote:
> Probably use /// style for consistency?

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86Base.h:716: void lowerIcmpAndBr(const InstIcmp *Icmp,
const InstBr *Br = nullptr);
On 2015/10/16 13:58:10, stichnot wrote:
> You're always calling this with 2 explicit args, so remove the default
> parameter?

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:204: (getConsumerKind(&Instr) != CK_Br && //
Icmp64 only folds in branches
On 2015/10/16 13:58:11, stichnot wrote:
> 80-col

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2657: lowerIcmpAndBr(Inst, nullptr);
On 2015/10/16 13:58:11, stichnot wrote:
> Maybe use "constexpr InstBr *Br = nullptr;" for clarity?

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2668: assert((Br == nullptr) && "not ready to
fold vector compare/branch");
On 2015/10/16 13:58:11, stichnot wrote:
> Make this a report_fatal_error() instead?
> 
> Also, I can't see that we would *ever* be doing branch optimizations on bool
> vector operations, so rephrase the error not to get people's hopes up? :)

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/src/IceTargetLowerin...
src/IceTargetLoweringX86BaseImpl.h:2916: _br(Traits::TableIcmp64[Index].C3,
Br->getTargetTrue(), Br->getTargetFalse());
On 2015/10/16 13:58:11, stichnot wrote:
> 80-col

Done.

https://chromiumcodereview.appspot.com/1407143002/diff/1/tests_lit/llvm2ice_t...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (left):

https://chromiumcodereview.appspot.com/1407143002/diff/1/tests_lit/llvm2ice_t...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:1019: ; CHECK: je
On 2015/10/16 13:58:11, stichnot wrote:
> Here and below, I think it would be better to replace this je with jne (or
> whatever), instead of deleting the check.

I've tightened the patterns for both O2 and Om1.

[Jim Stichnoth, 2015-10-16 19:39:05]

lgtm

[sehr, 2015-10-16 20:23:21]

Committed patchset #3 (id:40001) manually as
d981025a26d962d50f7fead809b44d16be9051f3 (presubmit successful).