Support unaligned integer loads on ARM and MIPS.

Support unaligned integer loads on ARM and MIPS.

Fixes SDK build with SIMARM/SIMARM64/SIMMIPS.

R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/26245de7a560de03faf382ec5b94fdff2b96e8c1

[rmacnak, 2016-10-25 20:28:47]

Description was changed from

==========
WIP unaligned access
==========

to

==========
Support for unaligned integer loads on ARM and MIPS.
==========

[rmacnak, 2016-10-25 20:36:04]

Description was changed from

==========
Support for unaligned integer loads on ARM and MIPS.
==========

to

==========
Support unaligned integer loads on ARM and MIPS.
==========

[rmacnak, 2016-10-25 23:37:20]

Description was changed from

==========
Support unaligned integer loads on ARM and MIPS.
==========

to

==========
Support unaligned integer loads on ARM and MIPS.

Fixes SDK build with SIMARM/SIMARM64/SIMMIPS.
==========

[rmacnak, 2016-10-25 23:37:34]

rmacnak@google.com changed reviewers:
+ johnmccutchan@google.com, zra@google.com

[zra, 2016-10-26 06:59:41]

Nice!

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm.h:1003: 
extra newline

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3696: break;
I'd sort of prefer leaving aligned_ out of the initializer list, and setting it
in a default case here. Otherwise maybe put a comment above the switch
explaining what it is for.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3740: break;
ditto

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1259: Address element_address(IP);
Can IP be kNoRegister instead, or maybe pass a bogus offset that the aligned()
case will overwrite? That way we can assert in the aligned cases below that
element_address wasn't used for something by mistake.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1322: __ ldr(result, element_address);
Likewise, maybe assert here that address == kNoRegister, or above before we
start doing the loads, something like:

ASSERT(aligned() ? (address == kNoRegister) : (element_address.something() ==
something));

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1434: intptr_t kNumTemps = needs_base ?
1 : 0;
Maybe:

const intptr_t kNumTemps = aligned() ? (needs_base ? 1 : 0) : 2;

so that we can follow the pattern and leave kNumTemps const.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1508: Address element_address(IP);
Same comments here about asserts as above.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:1076: Address element_address(TMP);
ditto

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:1342: Address element_address(TMP);
ditto

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1273: Address element_address(TMP);
ditto

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1536: Address element_address(TMP);
ditto

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1563: __ Comment("KArrayCid");
Leftover debug print?

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/unit_test.h#...
runtime/vm/unit_test.h:182: // TODO(zra): Supply more macros for 64-bit as tests
are added for ARM64.
We can probably remove this TODO =)

[Cutch, 2016-10-26 09:00:29]

LGTM w/c

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3696: break;
On 2016/10/26 06:59:41, zra wrote:
> I'd sort of prefer leaving aligned_ out of the initializer list, and setting
it
> in a default case here. Otherwise maybe put a comment above the switch
> explaining what it is for.

I'd like to see the switch statement factored into a separate function which
returns true/false.

Something like:

bool AddressWillBeAligned(intptr_t cid, bool default_value) { ... }

Then you could put it into initializer list:

aligned_(AddressWillBeAligned(class_id, aligned)),

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3850: bool aligned,
Consider defining an enum:

enum MemoryAccessKind {
  kAlignedMemoryAccess,
  kUnalignedMemoryAccess,
}

instead of passing true / false.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1434: intptr_t kNumTemps = needs_base ?
1 : 0;
On 2016/10/26 06:59:41, zra wrote:
> Maybe:
> 
> const intptr_t kNumTemps = aligned() ? (needs_base ? 1 : 0) : 2;
> 
> so that we can follow the pattern and leave kNumTemps const.

+1

[siva, 2016-10-26 10:37:20]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-10-26 10:37:21]

I assume you had to do this because the app-jit snapshot
generated using the simulator had unaligned loads in it,
does this mean we will have potentially less performant
code having generated an app-jit snapshot using the
simulator?

[rmacnak, 2016-10-31 22:57:26]

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm.h:1003: 
On 2016/10/26 06:59:41, zra wrote:
> extra newline

Done.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3696: break;
On 2016/10/26 09:00:29, Cutch wrote:
> On 2016/10/26 06:59:41, zra wrote:
> > I'd sort of prefer leaving aligned_ out of the initializer list, and setting
> it
> > in a default case here. Otherwise maybe put a comment above the switch
> > explaining what it is for.
> 
> I'd like to see the switch statement factored into a separate function which
> returns true/false.
> 
> Something like:
> 
> bool AddressWillBeAligned(intptr_t cid, bool default_value) { ... }
> 
> Then you could put it into initializer list:
> 
> aligned_(AddressWillBeAligned(class_id, aligned)),

Switched to enum and separate narrowing function.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1259: Address element_address(IP);
On 2016/10/26 06:59:41, zra wrote:
> Can IP be kNoRegister instead, or maybe pass a bogus offset that the aligned()
> case will overwrite? That way we can assert in the aligned cases below that
> element_address wasn't used for something by mistake.

Using kNoRegister.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1434: intptr_t kNumTemps = needs_base ?
1 : 0;
On 2016/10/26 06:59:41, zra wrote:
> Maybe:
> 
> const intptr_t kNumTemps = aligned() ? (needs_base ? 1 : 0) : 2;
> 
> so that we can follow the pattern and leave kNumTemps const.

Done.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1563: __ Comment("KArrayCid");
On 2016/10/26 06:59:41, zra wrote:
> Leftover debug print?

Removed.

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2452453002/diff/40001/runtime/vm/unit_test.h#...
runtime/vm/unit_test.h:182: // TODO(zra): Supply more macros for 64-bit as tests
are added for ARM64.
On 2016/10/26 06:59:41, zra wrote:
> We can probably remove this TODO =)

Done.

[rmacnak, 2016-10-31 22:59:03]

On 2016/10/26 10:37:21, siva wrote:
> I assume you had to do this because the app-jit snapshot
> generated using the simulator had unaligned loads in it,
> does this mean we will have potentially less performant
> code having generated an app-jit snapshot using the
> simulator?

The change applies generally to ARM/ARM64/MIPS, not just the simulator and not
just app snapshots.

[rmacnak, 2016-11-01 00:41:58]

Description was changed from

==========
Support unaligned integer loads on ARM and MIPS.

Fixes SDK build with SIMARM/SIMARM64/SIMMIPS.
==========

to

==========
Support unaligned integer loads on ARM and MIPS.

Fixes SDK build with SIMARM/SIMARM64/SIMMIPS.

R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/26245de7a560de03faf382ec5b94fdff2b96e8c1
==========

[rmacnak, 2016-11-01 00:42:00]

Committed patchset #4 (id:60001) manually as
26245de7a560de03faf382ec5b94fdff2b96e8c1 (presubmit successful).