[arm64] Fix pre-shifted immediate generation involving csp.

[arm64] Fix pre-shifted immediate generation involving csp.

The function that generated a pre-shifted immediate didn't account for the
instruction with post-shift being unencodable. Fix this by passing
information about the target instruction, and use it to limit the application
of pre-shift.

BUG=chromium:725858
Change-Id: Ia0f70b2ea057975d90162aa6889f15b553acd321
Review-Url: https://codereview.chromium.org/2922173004
Cr-Commit-Position: refs/heads/master@{#45911}
Committed: https://chromium.googlesource.com/v8/v8/+/849a08b8712470fae420dc7a804f552b9cb1a76a

[martyn.capewell, 2017-06-06 13:30:15]

martyn.capewell@arm.com changed reviewers:
+ bmeurer@chromium.org, tebbi@chromium.org

[Tobias Tebbi, 2017-06-08 10:22:30]

The CQ bit was checked by tebbi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 10:22:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tobias Tebbi, 2017-06-08 10:27:17]

On 2017/06/06 13:30:16, martyn.capewell wrote:

Thank you! The patch looks good, but I get crashes when I run
cctest/test-assembler-arm64/bic and
cctest/test-assembler-arm64/preshift_immediates locally (with the simulator) in
a debug build.

[Tobias Tebbi, 2017-06-08 10:27:51]

https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm64.cc:7173: __ Add(x1, x0, 0x1f7de);
How are these constants encodable with a shift? They do not have trailing zeros.

[martyn.capewell, 2017-06-08 10:37:17]

https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm64.cc:7173: __ Add(x1, x0, 0x1f7de);
On 2017/06/08 10:27:51, Tobias Tebbi wrote:
> How are these constants encodable with a shift? They do not have trailing
zeros.

In order to improve "encodability", ones are inserted on left shifting the
target immediate, which are removed again on right shift. So 0xffffff1 becomes:
 mov w16, #0xffffff1f
 add w2, w0, w16, lsr #4

0xffffff1f is encodable as a logical immediate, but 0xffffff10 is not.

The idea is to create a string of bits that wrap around the target register,
such that the rotation applied in logical immediate generation is more
effective.

[commit-bot: I haz the power, 2017-06-08 10:49:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 10:49:08]

Dry run: This issue passed the CQ dry run.

[Tobias Tebbi, 2017-06-08 14:27:25]

On 2017/06/08 10:37:17, martyn.capewell wrote:
>
https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
> File test/cctest/test-assembler-arm64.cc (right):
> 
>
https://codereview.chromium.org/2922173004/diff/1/test/cctest/test-assembler-...
> test/cctest/test-assembler-arm64.cc:7173: __ Add(x1, x0, 0x1f7de);
> On 2017/06/08 10:27:51, Tobias Tebbi wrote:
> > How are these constants encodable with a shift? They do not have trailing
> zeros.
> 
> In order to improve "encodability", ones are inserted on left shifting the
> target immediate, which are removed again on right shift. So 0xffffff1
becomes:
>  mov w16, #0xffffff1f
>  add w2, w0, w16, lsr #4
> 
> 0xffffff1f is encodable as a logical immediate, but 0xffffff10 is not.
> 
> The idea is to create a string of bits that wrap around the target register,
> such that the rotation applied in logical immediate generation is more
> effective.

Thanks, but I still don't see a test checking the case with a right-shifted
immediate due to (more than 4) trailing zeros, like in the mjsunit test case.

I understand now why the test case is crashing in debug builds:
The Mov() macro uses AssertStackConsistency(), which tries to load from csp.
This then leads to a crash.
Adding 
__ set_emit_debug_code(false);
in the beginning of the test case should solve this issue. Could you also add
this to cctest/test-assembler-arm64/bic?

[martyn.capewell, 2017-06-08 14:47:26]

On 2017/06/08 14:27:25, Tobias Tebbi wrote:

> Thanks, but I still don't see a test checking the case with a right-shifted
> immediate due to (more than 4) trailing zeros, like in the mjsunit test case.

I'll add another test.

> I understand now why the test case is crashing in debug builds:
> The Mov() macro uses AssertStackConsistency(), which tries to load from csp.
> This then leads to a crash.
> Adding 
> __ set_emit_debug_code(false);
> in the beginning of the test case should solve this issue. Could you also add
> this to cctest/test-assembler-arm64/bic?

I have a fix for this that involves switching the stack pointer to jssp before
touching csp.

[martyn.capewell, 2017-06-13 09:37:16]

Ping - does the updated patch fix the problem you're seeing, Tobias?

[Tobias Tebbi, 2017-06-13 14:37:22]

On 2017/06/13 09:37:16, martyn.capewell wrote:
> Ping - does the updated patch fix the problem you're seeing, Tobias?

Sorry, didn't see the patch. Yes, thanks a lot, looks perfect. LGTM

[Tobias Tebbi, 2017-06-13 14:37:29]

The CQ bit was checked by tebbi@chromium.org

[commit-bot: I haz the power, 2017-06-13 14:37:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 15:04:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497364649806840,
"parent_rev": "c87830200612f9d0c68d3a356fbc06cfa112afd7", "commit_rev":
"849a08b8712470fae420dc7a804f552b9cb1a76a"}

[commit-bot: I haz the power, 2017-06-13 15:04:23]

Description was changed from

==========
[arm64] Fix pre-shifted immediate generation involving csp.

The function that generated a pre-shifted immediate didn't account for the
instruction with post-shift being unencodable. Fix this by passing
information about the target instruction, and use it to limit the application
of pre-shift.

BUG=chromium:725858

Change-Id: Ia0f70b2ea057975d90162aa6889f15b553acd321
==========

to

==========
[arm64] Fix pre-shifted immediate generation involving csp.

The function that generated a pre-shifted immediate didn't account for the
instruction with post-shift being unencodable. Fix this by passing
information about the target instruction, and use it to limit the application
of pre-shift.

BUG=chromium:725858

Change-Id: Ia0f70b2ea057975d90162aa6889f15b553acd321
Review-Url: https://codereview.chromium.org/2922173004
Cr-Commit-Position: refs/heads/master@{#45911}
Committed:
https://chromium.googlesource.com/v8/v8/+/849a08b8712470fae420dc7a804f552b9cb...
==========

[commit-bot: I haz the power, 2017-06-13 15:04:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/849a08b8712470fae420dc7a804f552b9cb...