Subzero: Avoid unused insts for ARM Om1 lowering for arithmetic

Fix ARM Om1 lowering for arithmetic, and test.

The original arithmetic lowering was introducing some
unused mov instructions from legalization (e.g., the upper
part of shift num bits -- which should be 0 anyway), and
div helper calls don't actually use the legalized
parameters (handled separately by lowerCall). These unused
instructions cause the Om1 allocator to assert that LRBegin
exists but LREnd does not.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=kschimpf@google.com

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

[jvoung (off chromium), 2015-07-01 19:01:27]

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

[Karl, 2015-07-06 18:16:43]

lgtm

[jvoung (off chromium), 2015-07-06 21:01:29]

Committed patchset #6 (id:100001) manually as
70fa5255857bc3df97139924f61dec30ed9e7a4c (presubmit successful).

[Jim Stichnoth, 2015-07-07 11:50:29]

https://codereview.chromium.org/1210073017/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1210073017/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1110: // liveness analysis under -Om1 setting.
Hmm, I wonder if that Om1 validation check should be relaxed?  It checks that
any temporaries with a use in this block also have a definition in this block,
which seems sound, but it also checks that any temporary definition also has a
last use, which fails on this situation.  We could just DCE such definitions.

On the other hand, these checks enforce a discipline of not creating unused
assignments, making codegen and analysis more efficient...

[jvoung (off chromium), 2015-07-07 18:57:53]

On Tue, Jul 7, 2015 at 4:50 AM, <stichnot@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1210073017/diff/100001/src/IceTargetLoweringA...
> File src/IceTargetLoweringARM32.cpp (right):
>
>
>
https://codereview.chromium.org/1210073017/diff/100001/src/IceTargetLoweringA...
> src/IceTargetLoweringARM32.cpp:1110: // liveness analysis under -Om1
> setting.
> Hmm, I wonder if that Om1 validation check should be relaxed?  It checks
> that any temporaries with a use in this block also have a definition in
> this block, which seems sound, but it also checks that any temporary
> definition also has a last use, which fails on this situation.  We could
> just DCE such definitions.
>

True, that seems possible to relax.


>
> On the other hand, these checks enforce a discipline of not creating
> unused assignments, making codegen and analysis more efficient...
>
> https://codereview.chromium.org/1210073017/


Right. Since liveness happened to catch these extra instructions, I figured
we might as avoid unused insts to keep codegen and analysis efficient. ARM
already has more legalize-to-reg instructions than X86 so that may be a
good thing. The downside is moving the legalize calls around and possibly
having multiple invocations =(

Maybe the liveness assert could be clarified, factored out, or somehow
changed but we can discuss more =)

-- 
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 http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.