Clean up handling of ARM IR instruction "mov".

Clean up handling of ARM IR instruction "mov".

Also adds VMOVSR instruction to the integrated ARM assembler.

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

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

[Karl, 2016-01-15 23:52:40]

Description was changed from

==========
Clean up handling of ARM IR instruction "mov".

BUG=None
==========

to

==========
Clean up handling of ARM IR instruction "mov".

Also adds VMOVSR instruction to the integrated ARM assembler.

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

[Karl, 2016-01-15 23:57:31]

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

[John, 2016-01-16 00:24:13]

https://codereview.chromium.org/1596613002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1596613002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1019: const EmitForm Form) const {
I understand what you're doing, but it seems to me that this change makes things
harder to follow. Every method (with the exception of emiUsingForm) is
essentially initializing variables, then going about to do completely different
things for the different output types.

I believe it would be better if, instead of merging the methods, you created
helper functions with common logic (eg, IsVmove), and called them in separate
functions for textual, and binary outputs.

This cl also increases the minimum build, for the compiler will not be able to
DCE the prolog to these functions.

[Karl, 2016-01-19 18:32:39]

https://codereview.chromium.org/1596613002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1596613002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1019: const EmitForm Form) const {
On 2016/01/16 00:24:13, John wrote:
> I understand what you're doing, but it seems to me that this change makes
things
> harder to follow. Every method (with the exception of emiUsingForm) is
> essentially initializing variables, then going about to do completely
different
> things for the different output types.
> 
> I believe it would be better if, instead of merging the methods, you created
> helper functions with common logic (eg, IsVmove), and called them in separate
> functions for textual, and binary outputs.
> 
> This cl also increases the minimum build, for the compiler will not be able to
> DCE the prolog to these functions.

Ok. For the moment, I will not try to merge code between the text and binary
forms. Refactoring to match this change.

[Jim Stichnoth, 2016-01-19 21:14:58]

https://codereview.chromium.org/1596613002/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1596613002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1123: // TODO(kschimpf) Handle moving contants into
registers.
constants

[Karl, 2016-01-21 16:05:47]

https://codereview.chromium.org/1596613002/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1596613002/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1123: // TODO(kschimpf) Handle moving contants into
registers.
On 2016/01/19 21:14:57, stichnot wrote:
> constants

Done.

[Jim Stichnoth, 2016-01-22 04:36:06]

LGTM, assuming it LGT John.

[native-client-reviews_googlegroups.com, 2016-01-22 15:30:50]

John,

Could you please review this CL. Thanks.

https://codereview.chromium.org/1596613002/

Karl Schimpf

On Thu, Jan 21, 2016 at 8:36 PM, <stichnot@chromium.org> wrote:

> LGTM, assuming it LGT John.
>
> https://codereview.chromium.org/1596613002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[John, 2016-01-22 23:03:45]

lgtm

[Karl, 2016-01-22 23:15:52]

Description was changed from

==========
Clean up handling of ARM IR instruction "mov".

Also adds VMOVSR instruction to the integrated ARM assembler.

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

to

==========
Clean up handling of ARM IR instruction "mov".

Also adds VMOVSR instruction to the integrated ARM assembler.

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

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

[Karl, 2016-01-22 23:15:54]

Committed patchset #6 (id:100001) manually as
4ff90be3620f73ddb41d062848325b22a6a0ac0e (presubmit successful).