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

arm64 is not a valid aot target. #3688

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

Andersbakken
Copy link
Contributor

Without this aot files won't load on arm64 builds on mac.

@yamt
Copy link
Collaborator

yamt commented Aug 5, 2024

can you explain a bit?

other people were saying it should be arm64.
see #1365

@Andersbakken
Copy link
Contributor Author

wamrc doesn't appear to have an arm64 target. (Based on calling wamrc --target=help)

I can also confirm that my m3 mac is able to run an aarch64.v8 aot with this change.

@yamt
Copy link
Collaborator

yamt commented Aug 6, 2024

wamrc doesn't appear to have an arm64 target. (Based on calling wamrc --target=help)

how about the default? (without --target option)

maybe we can make wamrc normalize it?
@eloparco how do you think?

@Andersbakken
Copy link
Contributor Author

Andersbakken commented Aug 6, 2024

Hm. I get this then:

wamrc -o pyramid.arm64.aot pyramid.wasm --help
Create AoT compiler with:
  target:        arm64
  target cpu:    apple-m1
  target triple: arm64-pc-linux-gnu
  cpu features:
  opt level:     3
  size level:    1
  output format: AoT file
LLVM ERROR: Only small, tiny and large code models are allowed on AArch64
zsh: abort      wamrc -o pyramid.arm64.aot pyramid.wasm --help

Which kinda implies that it tried to do that but it aborted. Not sure what the code model thing is about tbh. Didn't see anything about it in the help.

@yamt
Copy link
Collaborator

yamt commented Aug 6, 2024

Hm. I get this then:

wamrc -o pyramid.arm64.aot pyramid.wasm --help
Create AoT compiler with:
  target:        arm64
  target cpu:    apple-m1
  target triple: arm64-pc-linux-gnu
  cpu features:
  opt level:     3
  size level:    1
  output format: AoT file
LLVM ERROR: Only small, tiny and large code models are allowed on AArch64
zsh: abort      wamrc -o pyramid.arm64.aot pyramid.wasm --help

Which kinda implies that it tried to do that but it aborted. Not sure what the code model thing is about tbh. Didn't see anything about it in the help.

i agree it's a bit cryptic. it means --size-level.
see #3689

@Andersbakken
Copy link
Contributor Author

While it claimed to create that format based on stdout I still get the same error when trying to load it (after reverting my patch). I think it just plain doesn't exist as a target. Mac is just aarch64 AFAICT.

AOT module load failed: invalid target type, expected arm64 but got aarch64v8

@yamt
Copy link
Collaborator

yamt commented Aug 7, 2024

While it claimed to create that format based on stdout I still get the same error when trying to load it (after reverting my patch). I think it just plain doesn't exist as a target. Mac is just aarch64 AFAICT.

what's "it"? can you share the exact command line option?

AOT module load failed: invalid target type, expected arm64 but got aarch64v8

my understanding is:
historically arm64 is used for mac.
llvm treats it as an alias of aarch64 in some places.

@Andersbakken
Copy link
Contributor Author

Sorry. That wasn't really clear.

My findings are that wasm-micro-runtime still says that this file is the wrong target type when loaded. E.g. The file appears to be aarch64 even when produced with the command line underneath.

main [src_kodak][abakken@b.local:~/dev/kodak/test/triangles-cpp] wamrc --size-level=3 -o triangles.arm64.aot ./triangles.wasm
Create AoT compiler with:
  target:        arm64
  target cpu:    apple-m1
  target triple: arm64-pc-linux-gnu
  cpu features:
  opt level:     3
  size level:    3
  output format: AoT file
Compile success, file triangles.arm64.aot was generated.

@Andersbakken
Copy link
Contributor Author

I think I made a mistake in testing. It does indeed load it correctly now like you thought it would. I guess the only problem is that wamrc needs to get another valid --target. I'll see if I can come up with a pr for that. Sorry about the confusion.

@yamt
Copy link
Collaborator

yamt commented Aug 8, 2024

i think the reported problem is still valid because there seems no way to cross-build "arm64" aot module.
the following is on x86-64 macOS.

spacetanuki% ./wamrc --target=arm64 -o aot ~/git/garbage/wasm/hello/hello.wasm 
Error: Invalid target. Use --target=help to list all supported targets
spacetanuki% ./wamrc --target=aarch64 -o aot ~/git/garbage/wasm/hello/hello.wasm
Create AoT compiler with:
  target:        aarch64v8
  target cpu:    
  target triple: aarch64v8-pc-linux-gnu
  cpu features:  
  opt level:     3
  size level:    3
  output format: AoT file
Compile success, file aot was generated.
spacetanuki% 

IMO, we should

  • make wamrc normalize "arm64" to "aarch64v8".
  • make aot_loader accept "aarch64v8" as your patch does. (and make it accept "arm64" as well if we care compatibility)

@eloparco how do you think? (i'm asking you because you made the "use arm64 for apple" change.)

@Andersbakken
Copy link
Contributor Author

I guess it maybe was premature to close this pr. @yamt suggestions sound good to me. I've tried to update the patch to implement it that way. Maybe @eloparco could take a look?

I have verified that I can do the following now:

  1. Run aot files on mac (m3) produced with default parameters with wamrc on mac
  2. Run aot files on mac (m3) produced with --target=aarch64v8 with wamrc on any platform
  3. Run aot files on mac(m3) produced with default parameters with previous version of wamrc on mac

@Andersbakken
Copy link
Contributor Author

@wenyongh @yamt Would it be okay to land this?

@@ -546,6 +546,12 @@ load_target_info_section(const uint8 *buf, const uint8 *buf_end,
return false;
}

#if (defined(__APPLE__) || defined(__MACH__)) && defined(__arm64__)
// for backwards compatibility with previous wamrc aot files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use /* .. */ to comment the code like other places? This is C source code, we usually add comment like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe we can remove the macro control (L549)? Just convert target arm64 to aarch64v8 unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with both points. Will update.

core/iwasm/compilation/aot_llvm.c Show resolved Hide resolved
wamr-compiler/main.c Show resolved Hide resolved
@wenyongh
Copy link
Contributor

@wenyongh @yamt Would it be okay to land this?

@Andersbakken I tried to look into the code, if IIUC, what the PR does are: (1) wamrc accepts --target=arm64, and emits target aarch64v8 in the aot file instead, (2) aot loader accepts arm64 in aot file for backward compatibility, and converts it into aarch64v8 instead, and it also sets host default arch to aarch64v8 when BUILD_TARGET macro is AARCH64, so check_machine_info runs succeeded no matter the input target in aot file is aarch64v8 or arm64. (3) change size-level to 3 when host is macos arm64.

I added several comments, could you help have a check? Thanks.

@Andersbakken
Copy link
Contributor Author

@wenyongh @yamt Would it be okay to land this?

@Andersbakken I tried to look into the code, if IIUC, what the PR does are: (1) wamrc accepts --target=arm64, and emits target aarch64v8 in the aot file instead, (2) aot loader accepts arm64 in aot file for backward compatibility, and converts it into aarch64v8 instead, and it also sets host default arch to aarch64v8 when BUILD_TARGET macro is AARCH64, so check_machine_info runs succeeded no matter the input target in aot file is aarch64v8 or arm64. (3) change size-level to 3 when host is macos arm64.

I added several comments, could you help have a check? Thanks.

  1. The intention was actually to try to get rid of arm64 as a valid target since it never was supported before but otherwise your summary sounds right.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

core/iwasm/compilation/aot_llvm.c Show resolved Hide resolved
@Andersbakken
Copy link
Contributor Author

LGTM

Thanks. I'll add the comment

Make wamrc normalize "arm64" to "aarch64". Previously the only way to
make the "arm64" target was to not specify a target on 64 bit
arm-based mac builds. Now arm64 and aarch64 are treated as the same.

Make aot_loader accept "aarch64v8" on arm-based apple (as well as
accepting legacy "arm64" based aot targets).

This also removes __APPLE__ and __MACH__ from the block that defaults
size_level to 1 since
@wenyongh wenyongh merged commit e8c2952 into bytecodealliance:main Aug 23, 2024
387 checks passed
LazyBoy-KK pushed a commit to LazyBoy-KK/wasm-micro-runtime that referenced this pull request Oct 14, 2024
Make wamrc normalize "arm64" to "aarch64v8". Previously the only way to
make the "arm64" target was to not specify a target on 64 bit arm-based
mac builds. Now arm64 and aarch64v8 are treated as the same.

Make aot_loader accept "aarch64v8" on arm-based apple (as well as
accepting legacy "arm64" based aot targets).

This also removes __APPLE__ and __MACH__ from the block that defaults
size_level to 1 since it doesn't seem to be supported for aarch64:
`LLVM ERROR: Only small, tiny and large code models are allowed on AArch64`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants