[turbofan] Add alignment parameter to StackSlot operator

[turbofan] Add alignment parameter to StackSlot operator

If alignment parameter is set, the memory returned by the
StackSlot operator will be aligned according to the parameter.

The implementation goes like this. If alignment parameter is set
we allocate a bit more memory than actually needed and so we
can move the beginning of the StackSlot in order to have it aligned.


BUG=

Review-Url: https://codereview.chromium.org/2816743003
Cr-Commit-Position: refs/heads/master@{#45197}
Committed: https://chromium.googlesource.com/v8/v8/+/d8bfdb7a998adadc56aa5705a5998e75ceae7675

[ivica.bogosavljevic, 2017-04-13 13:50:48]

Description was changed from

==========
[turbofan] Add alignment parameter to StackSlot operator

BUG=
==========

to

==========
[turbofan] Add alignment parameter to StackSlot operator

If alignment parameter is set, the memory returned by the
StackSlot operator will be aligned according to the parameter.

The implementation goes like this. If alignment parameter is set
we allocate a bit more memory than actually needed and so we
can move the beginning of the StackSlot in order to have it aligned.


BUG=
==========

[ivica.bogosavljevic, 2017-04-13 13:50:48]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ titzer@chromium.org

[ivica.bogosavljevic, 2017-04-13 13:52:01]

PTAL.

There is a reference implementation for MIPS32. If architecture does not align
stack slot properly, no harm is done.

This is needed in order to fix forwarding of WASM parameters to WASM interpreter
(https://codereview.chromium.org/2705293011/)

[titzer, 2017-04-13 15:35:31]

titzer@chromium.org changed reviewers:
+ ahaas@chromium.org
- titzer@chromium.org

[ivica.bogosavljevic, 2017-04-18 07:37:39]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ titzer@chromium.org

[ivica.bogosavljevic, 2017-04-18 07:37:40]

PTAL

[ivica.bogosavljevic, 2017-04-20 08:22:17]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 08:22:25]

Dry run: 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-04-20 08:48:31]

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

[commit-bot: I haz the power, 2017-04-20 08:48:31]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-04-24 10:35:55]

ahaas@chromium.org changed reviewers:
+ clemensh@chromium.org

[ahaas, 2017-04-24 11:43:54]

Could you add tests for this CL, e.g. to cctest/test-run-machops.cc? It is also
not clear to me how this alignment parameter will be used, and if it affects
other platforms.

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:114: int AllocateSpillSlot(int width, int alignement = 0) {
alignment

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/instructio...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:1890: void
InstructionSelector::VisitStackSlot(Node* node) {
Can you move this code to platform-specific code? This CL could make the stack
frames of other platforms bigger than necessary, as far as I understand the
code.

[ivica.bogosavljevic, 2017-04-27 13:56:57]

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/frame.h#ne...
src/compiler/frame.h:114: int AllocateSpillSlot(int width, int alignement = 0) {
On 2017/04/24 11:43:54, ahaas wrote:
> alignment

Acknowledged.

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/instructio...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2816743003/diff/20001/src/compiler/instructio...
src/compiler/instruction-selector.cc:1890: void
InstructionSelector::VisitStackSlot(Node* node) {
On 2017/04/24 11:43:54, ahaas wrote:
> Can you move this code to platform-specific code? This CL could make the stack
> frames of other platforms bigger than necessary, as far as I understand the
> code.

Acknowledged.

[ivica.bogosavljevic, 2017-04-28 12:27:33]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 12:27:44]

Dry run: CQ is trying da patch.

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

[ivica.bogosavljevic, 2017-05-04 08:22:30]

Added MIPS64 implementation, added regression tests. PTAL

[ahaas, 2017-05-08 08:58:36]

ahaas@chromium.org changed reviewers:
+ bmeurer@chromium.org

[ahaas, 2017-05-08 08:58:37]

lgtm with nits.

Benedikt, can you take a look as an owner of the compiler directory?

https://codereview.chromium.org/2816743003/diff/80001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2816743003/diff/80001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:6745:
RawMachineAssemblerTester<int32_t> r(MachineType::Int32());
I think you don't need a parameter for this test, do you? If you don't need the
parameter, then you can just use
RawMachineAssemblerTester<int32_t> r;
and then call r.Call() below.

https://codereview.chromium.org/2816743003/diff/80001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:6751: int alignment_count =
sizeof(alignments) / sizeof(int);
You can just use arraysize(alignments).

[Benedikt Meurer, 2017-05-08 09:03:24]

LGTM (rubber-stamped)

[ivica.bogosavljevic, 2017-05-09 11:36:35]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 11:36:42]

Dry run: 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-05-09 12:03:22]

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

[commit-bot: I haz the power, 2017-05-09 12:03:23]

Dry run: This issue passed the CQ dry run.

[ivica.bogosavljevic, 2017-05-09 14:23:48]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2017-05-09 14:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2816743003/#ps120001
(title: "Small fixes in test")

[commit-bot: I haz the power, 2017-05-09 14:23:54]

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-05-09 14:25:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494339828772990,
"parent_rev": "1c1e700ccde5d9a1229d945a27f6651ffd146cbb", "commit_rev":
"d8bfdb7a998adadc56aa5705a5998e75ceae7675"}

[commit-bot: I haz the power, 2017-05-09 14:25:44]

Description was changed from

==========
[turbofan] Add alignment parameter to StackSlot operator

If alignment parameter is set, the memory returned by the
StackSlot operator will be aligned according to the parameter.

The implementation goes like this. If alignment parameter is set
we allocate a bit more memory than actually needed and so we
can move the beginning of the StackSlot in order to have it aligned.


BUG=
==========

to

==========
[turbofan] Add alignment parameter to StackSlot operator

If alignment parameter is set, the memory returned by the
StackSlot operator will be aligned according to the parameter.

The implementation goes like this. If alignment parameter is set
we allocate a bit more memory than actually needed and so we
can move the beginning of the StackSlot in order to have it aligned.


BUG=

Review-Url: https://codereview.chromium.org/2816743003
Cr-Commit-Position: refs/heads/master@{#45197}
Committed:
https://chromium.googlesource.com/v8/v8/+/d8bfdb7a998adadc56aa5705a5998e75cea...
==========

[commit-bot: I haz the power, 2017-05-09 14:25:45]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/d8bfdb7a998adadc56aa5705a5998e75cea...

[Michael Achenbach, 2017-05-09 16:13:34]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2867403002/ by machenbach@chromium.org.

The reason for reverting is: Seems to break cfi:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20cfi/builds....