Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow gen_snapshot to generate instructions snapshots for multiple architectures. #31709

Open
chinmaygarde opened this Issue Dec 22, 2017 · 15 comments

Comments

Projects
None yet
9 participants
@chinmaygarde
Copy link
Member

chinmaygarde commented Dec 22, 2017

Today, the x86_64 host gen_snapshot can only generate the AOT instructions snapshots for aarch64 and x86_64 (with a different version of gen_snapshot for each target). Similarly, an i386 gen_shapshot is required for arm-v7a instructions. This requires the tooling the ship (and the users to download) a target specific version of gen_snapshot. It also adds a lot of complicated logic in the tooling to select the gen_snapshot for each host-target pair.

It would be great if a single gen_shapshot were able to generate instructions for multiple target architectures. Fewer variants of the same target would need to be build, tested, uploaded to the cloud and downloaded by our users.

@chinmaygarde

This comment has been minimized.

Copy link
Member Author

chinmaygarde commented Dec 22, 2017

CC @a-siva

@a-siva

This comment has been minimized.

Copy link
Contributor

a-siva commented Dec 22, 2017

@zanderso

This comment has been minimized.

Copy link
Member

zanderso commented Dec 22, 2017

Since this would entail a significant refactoring of the VM, to help prioritize this, it would be good to have an estimate of what the percentage savings would be relative to all the other bytes that a Flutter end-user has to pull down.

@mraleph

This comment has been minimized.

Copy link
Contributor

mraleph commented Dec 22, 2017

This would be rather easy to do after the refactoring planed for the compilation pipeline where we aim to separate compiler from the rest of the VM. Dart 2 is currently taking priority but I would expect us to be able to get back to it closer to the end of Q1.

@keertip

This comment has been minimized.

Copy link
Contributor

keertip commented Nov 5, 2018

@mraleph, now that Dart 2 is out and well, is this something that can be worked on. We are starting to look at building gen_snapshot internally, and it would be so much easier to build just one flavor of the binary.

@mraleph

This comment has been minimized.

Copy link
Contributor

mraleph commented Nov 5, 2018

I am working on enabling generating arm32 snapshots with 64-bit gen_snapshot binary this quarter.

I think this would lay a lot of the ground work needed for having just a single gen_snapshot - but whether we can implement this or not depends on how much the first step takes.

@mehmetf

This comment has been minimized.

Copy link

mehmetf commented Dec 5, 2018

This just became high priority for Google.

We can't build Flutter engine in Google without this change because the internal toolchain no longer allows building x86 binaries.

@mehmetf

This comment has been minimized.

Copy link

mehmetf commented Dec 5, 2018

Just to be clear, the ability to generate arm32 snapshots with 64-bit gen_snapshot is the high priority task. We can live with the fact that android vs ios requires different binaries.

@mehmetf

This comment has been minimized.

Copy link

mehmetf commented Dec 6, 2018

What happens today when I try to build gen_snapshot and pass

    "TARGET_ARCH_ARM",
    "TARGET_OS_ANDROID",

as defines.

runtime/platform/globals.h:310:2: error: Mismatched Host/Target architectures.
#error Mismatched Host/Target architectures.
 ^
1 error generated.
@mraleph

This comment has been minimized.

Copy link
Contributor

mraleph commented Dec 6, 2018

I have started working on this - but this is still at least a month away, if we want to do it the right way.

@eseidelGoogle

This comment has been minimized.

Copy link

eseidelGoogle commented Dec 11, 2018

@mraleph I spoke with @mehmetf just now just to double-check that this was the right thing for us to spend on. Between he and I we imagined a work-around by which we built the 32-bit binaries in another manner outside of the internal toolchain. The engineering to make that work would be non-zero (and a bit of a hack) so if you spending on this is good for the VM, we should do it. But if you feel this is wasted work to invest in we could look more deeply into the hacks mehemet and I discussed (which would be specific to the internal systems).

@eseidelGoogle

This comment has been minimized.

Copy link

eseidelGoogle commented Dec 11, 2018

@cbracken also reminds me that this would benefit mac as well, where Mojave now warns for 32-bit binaries (which gen_snapshot for older iOS phones is) and whatever is after Mojave would disallow running 32-bit binaries supposedly.

dart-bot pushed a commit that referenced this issue Jan 11, 2019

[vm] Decouple growable_array.h and zone.h from thread.h
- Introduce a slimmed down version of thread.h, which just depends on the
Zone and StackResource.
- Introduce a layering check that would prevent the coupling in the future.

This is the first step towards decoupling compiler from runtime.

There are multiple reasons to introduce the decoupling but the main
reason currently is to introduce a controlled surface through which
compiler reaches into runtime to catch any places where runtime word size
might influence the compiler and then enable building compiler that
targets 32-bit runtime but is embedded into a 64-bit runtime.

Issue #31709

Change-Id: Id63ebbaddca55dd097298e51c90d957a73fa476e
Reviewed-on: https://dart-review.googlesource.com/c/87182
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

dart-bot pushed a commit that referenced this issue Jan 25, 2019

[vm] Decouple assemblers from runtime.
This is the next step towards preventing compiler from directly peeking
into runtime and instead interact with runtime through a well defined
surface. The goal of the refactoring to locate all places where compiler
accesses some runtime information and partion those accesses into two
categories:

- creating objects in the host runtime (e.g. allocating strings, numbers, etc)
during compilation;
- accessing properties of the target runtime (e.g. offsets of fields) to
embed those into the generated code;

This change introduces dart::compiler and dart::compiler::target namespaces.

All code in the compiler will gradually be moved into dart::compiler namespace.
One of the motivations for this change is to be able to prevent access to
globally defined host constants like kWordSize by shadowing them in the
dart::compiler namespace.

The nested namespace dart::compiler::target hosts all information about
target runtime that compiler could access, e.g. compiler::target::kWordSize
defines word size of the target which will eventually be made different
from the host kWordSize (defined by dart::kWordSize).

The API for compiler to runtime interaction is placed into compiler_api.h.

Note that we still permit runtime to access compiler internals directly -
this is not going to be decoupled as part of this work.

Issue #31709

Change-Id: If4396d295879391becfa6c38d4802bbff81f5b20
Reviewed-on: https://dart-review.googlesource.com/c/90242
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

dart-bot pushed a commit that referenced this issue Feb 11, 2019

[vm] Decouple stub code from runtime.
This is the next step towards preventing compiler from directly peeking
into runtime and instead interact with runtime through a well defined
surface.

This CL decouples the hand-written stub codes from the runtime. The
target architecture dependent stubs are moved to
dart::compiler::StubCodeCompiler which use dart::compiler::target:*
for accessing any runtime related code.

The generation of type testing stubs is moved to separate files for the
time being.

Issue #31709

Change-Id: I1b4f1cca0acb704b30b80eca7f634734772389b5
Reviewed-on: https://dart-review.googlesource.com/c/92138
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Aart Bik <ajcbik@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>

dart-bot pushed a commit that referenced this issue Feb 12, 2019

Reland "[vm] Decouple stub code from runtime"
This is the next step towards preventing compiler from directly peeking
into runtime and instead interact with runtime through a well defined
surface.

This CL decouples the hand-written stub codes from the runtime. The
target architecture dependent stubs are moved to
dart::compiler::StubCodeCompiler which use dart::compiler::target:*
for accessing any runtime related code.

The generation of type testing stubs is moved to separate files for the
time being.

Issue #31709

Change-Id: Icd0995b18a7bac496b1e12231cf437943f5c94f1
Reviewed-on: https://dart-review.googlesource.com/c/92720
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
Auto-Submit: Martin Kustermann <kustermann@google.com>

dart-bot pushed a commit that referenced this issue Feb 13, 2019

[vm] Decouple intrinsifier code from runtime
This is the next step towards preventing compiler from directly peeking
into runtime and instead interact with runtime through a well defined
surface.

This CL decouples the hand-written intrinsifier code from the runtime:

  * the intrinsifier is split up into a GraphIntrinsifier and AsmIntrinsifier
  * the recognized methods list is moved to a separate .h file
  * all intrinsifier code is moved into dart::compiler namespace
  * the AsmIntrinsifier is only interacting with RT through runtime_api.h

Issue #31709

Change-Id: I0a73ad620e051dd49c9db7da3241212b3b74ccdd
Reviewed-on: https://dart-review.googlesource.com/c/92740
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Aart Bik <ajcbik@google.com>

dart-bot pushed a commit that referenced this issue Feb 22, 2019

[VM] Decouple compile_type.h from runtime by making it only use runti…
…me_api.h

Issue #31709

Change-Id: I36ae58a4d4f4540433cdb6de2aa370d025142863
Reviewed-on: https://dart-review.googlesource.com/c/93942
Auto-Submit: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
@mehmetf

This comment has been minimized.

Copy link

mehmetf commented Mar 2, 2019

There are a couple of projects in Google that are asking for this. I see PRs fly by. It would be great if we have an ETA. Is end of Q2 a reasonable estimate?

@dnfield

This comment has been minimized.

Copy link
Contributor

dnfield commented Mar 21, 2019

Apple announced at the last WWDC that Mojave would be the last macOS to support 32 bit. This could very well mean that we only have until September before this becomes a more serious problem.

@mraleph

This comment has been minimized.

Copy link
Contributor

mraleph commented Mar 22, 2019

I had discussions with @mkustermann about this issue and we arrived to the conclusion that our original approach would probably require to continue rather tedious refactoring for another quarter. It has various benefits (e.g. we essentially audit every single line in the compiler that interacts with runtime system and make sure that word size discrepancies are discovered) and we will also arrive to the setup in which a single gen_snapshot could be made to generate code for multiple target architectures.

However this would require way too much effort.

Instead we are going to try something much simpler, where we don't decouple runtime and compiler entirely and instead just audit backend for occurances of known constants and methods that depend on word size.

This should allow us to get to X64 gen_snapshot which can generate ARM32 code much faster.

I have started on this and it indeed seems that the progress could be much faster in this setup.

I will update this issue next week with the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.