This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][builtins] Support builtins for armv8m.base
ClosedPublic

Authored by fdischner on Feb 3 2023, 2:34 PM.

Details

Summary

This allows building the compiler builtins library for the Armv8-M
Baseline architecture. It can be built in the same way as other
baremetal targets using the appropriate '--target' flag
(e.g. --target=armv8m.base-eabi).

NOTE: As with the other Cortex-M targets, only the builtins library is supported. There is no support for sanitizers, etc.

The armv8m.base architecture is a superset of armv6m, so adding it to
the cmake files using thumb1_SOURCES is almost enough for it to compile.
Minor changes are needed to divsi3 and udivsi3, because armv8m.base does
have support for div instructions but not mov with an immediate operand.

Diff Detail

Event Timeline

fdischner created this revision.Feb 3 2023, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 2:34 PM
fdischner requested review of this revision.Feb 3 2023, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 2:34 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Looks OK to me. I think we can simplify by always using the movs instruction.

compiler-rt/lib/builtins/arm/divsi3.S
40

In this case I think it is better to use movs r0,#0 universally as it is a 16-bit instruction and mov r0, #0 is a 32-bit instruction.

As we don't use the flags before returning and as flags are not preserved across prodcedure call boundaries there isn't an advantage to use the larger instruction.

Perhaps

// Use movs for compatibility with v8-m.base.
movs r0, #0
compiler-rt/lib/builtins/arm/udivsi3.S
35

Same here, we can always use movs

fdischner updated this revision to Diff 495106.Feb 6 2023, 6:24 AM

As per review, simplify by always using movs.

fdischner marked 2 inline comments as done.Feb 6 2023, 6:29 AM
fdischner added inline comments.
compiler-rt/lib/builtins/arm/divsi3.S
40

Thanks, that was my initial thought as well, but I wasn't sure if there was some advantage to using mov that I wasn't aware of. I've updated the patch to simplify this.

peter.smith accepted this revision.Feb 6 2023, 7:00 AM

Thanks for the update, I'm happy with the changes. Best wait a bit to let anyone in the US timezone comment.

This revision is now accepted and ready to land.Feb 6 2023, 7:00 AM
MaskRay accepted this revision.Feb 6 2023, 10:26 AM

LGTM. IIUC this makes clang --target=armv8m.base-eabi -c compilable.

fdischner marked an inline comment as done.Feb 6 2023, 10:00 PM

LGTM. IIUC this makes clang --target=armv8m.base-eabi -c compilable.

I’m not sure I understand. Clang already supports compiling for the armv8m.base-eabi target. This just allows building the compiler-rt builtins library for this target. Or maybe that’s what you are saying?

Is there something that still needs to be done for this to be committed? I'm new to LLVM and don't have commit access, so could use some help here.

MaskRay added a comment.EditedFeb 15 2023, 5:24 PM

Is there something that still needs to be done for this to be committed? I'm new to LLVM and don't have commit access, so could use some help here.

You can provide your name and email address to be used in git commit --amend --author='... <...>' so that someone with commit access can push this patch for you.
Note: if you upload the patch via arc diff 'HEAD^' (https://llvm.org/docs/Phabricator.html), arc patch D143297 will obtain the name and email.

I think it'd be nice to adjust the commit message (Phabricator summary) to include information about how you configure compiler-rt and do the testing.
I was confused so I asked for confirmation that this fixed the build when someone uses clang --target=armv8m.base-eabi -c to assemble the file.

fdischner updated this revision to Diff 505316.Mar 14 2023, 4:26 PM

Updating commit message

fdischner edited the summary of this revision. (Show Details)Mar 14 2023, 4:37 PM

I've updated the commit message to hopefully clear up the confusion and also re-uploaded the patch using arc diff --update D143297. In case the author still isn't correct with arc patch D143297, please use "Frank Dischner <fdischner@google.com>" as the author. Thanks!

This revision was automatically updated to reflect the committed changes.