Stream: git-wasmtime

Topic: wasmtime / PR #10334 Generate x64 assembler definitions i...

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:05):

adambratschikaye opened PR #10334 from adambratschikaye:abk/move-assembler-x64-generation to bytecodealliance:main:

Generate assembler-definitions.isle during build of cranelift-codegen instead of cranelift-assembler-x64. The problem with generating it in cranelift-assembler-x64 is that that setup requires cranelift-codegen to have access to the OUT_DIR used by cranelift-assembler-x64 which is prevented when building under Bazel which tries to isolate the builds of separate crates.

This change instead adds a function to cranelift-assembler-x64 which returns the expected contents of assembler-definitions.isle so that the build step of cranelift-codegen can write it to an accessible directory.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:05):

adambratschikaye requested wasmtime-compiler-reviewers for a review on PR #10334.

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:05):

adambratschikaye requested fitzgen for a review on PR #10334.

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:18):

adambratschikaye updated PR #10334.

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:23):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Mar 05 2025 at 11:23):

bjorn3 created PR review comment:

I think it would be nice if this executable kept showing a path to a file on the disk. It is for debugging only and emitting the entire file to stdout would be annoying. Maybe it could write the file to somewhere in the directory from which it is invoked?

Wasmtime GitHub notifications bot (Mar 05 2025 at 12:30):

adambratschikaye updated PR #10334.

Wasmtime GitHub notifications bot (Mar 05 2025 at 16:10):

abrown requested abrown for a review on PR #10334.

Wasmtime GitHub notifications bot (Mar 05 2025 at 17:22):

abrown submitted PR review:

Thank you for submitting this change; I would like to suggest another approach here but I want to make it clear that this looks fine to me as a temporary measure. In thinking about generating the ISLE definitions, I realized recently that they are quite cranelift-codegen-specific. I think that, if we move the ISLE generation out of the cranelift-assembler-x64-meta crate and directly into the cranelift-codegen-meta crate, we not only fix this OUT_DIR issue that affects Bazel builds but it's better code organization. All ISLE compilation/generation lives in cranelift-codegen-meta already, so why not move the assembler part of that to the same place? I recognize that may be a larger refactor than this PR and I'm willing to do it since I'm most familiar with all of this. Thoughts?

Wasmtime GitHub notifications bot (Mar 05 2025 at 17:22):

abrown created PR review comment:

What's unfortunate about this change is that it loses information about the _other_ files generated for the assembler. There were three files and this forgets about two of them.

Wasmtime GitHub notifications bot (Mar 06 2025 at 07:05):

adambratschikaye updated PR #10334.

Wasmtime GitHub notifications bot (Mar 06 2025 at 07:15):

adambratschikaye submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 07:15):

adambratschikaye created PR review comment:

What about changing the main function back to its previous behavior (here)?

Wasmtime GitHub notifications bot (Mar 06 2025 at 07:17):

adambratschikaye commented on PR #10334:

I think that, if we move the ISLE generation out of the cranelift-assembler-x64-meta crate and directly into the cranelift-codegen-meta crate, we not only fix this OUT_DIR issue that affects Bazel builds but it's better code organization.

I'm not familiar enough with this part of the codebase to comment on whether it's better organization or not, but I agree it would fix the issue with Bazel builds.

Wasmtime GitHub notifications bot (Mar 07 2025 at 14:47):

abrown commented on PR #10334:

@adambratschikaye: ok, I pushed #10352 as an alternative to this PR. Can you confirm that it fixes the build issues you were seeing in Bazel?

Wasmtime GitHub notifications bot (Mar 10 2025 at 08:03):

adambratschikaye closed without merge PR #10334.

Wasmtime GitHub notifications bot (Mar 10 2025 at 08:03):

adambratschikaye commented on PR #10334:

@adambratschikaye: ok, I pushed #10352 as an alternative to this PR. Can you confirm that it fixes the build issues you were seeing in Bazel?

Yep, that fix works for us. Thanks for helping out!

Last updated: Apr 17 2025 at 03:17 UTC