This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] support armv4t
ClosedPublic

Authored by stuij on Nov 25 2022, 8:48 AM.

Details

Summary

The main thing that needed changing was excluding functionality that
isn't supported on armv4t. So excluding Arm specific builtin assembly files.

In the process some files were renamed and the source was annotated where
appropriate, so it's a bit easier to follow what group of files are meant for
what purpose.

Diff Detail

Event Timeline

stuij created this revision.Nov 25 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 8:48 AM
stuij requested review of this revision.Nov 25 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 8:48 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
stuij updated this revision to Diff 477989.Nov 25 2022, 9:18 AM

fix renaming typo

Will take a look next week. I think you should be able to get v5t support from the same changes as the overall Arm/Thumb instruction. That could be done in a follow up patch though.

Code changes LGTM. I've made some suggestions for the comments and names.

compiler-rt/lib/builtins/CMakeLists.txt
378

IIUC these are the sources that can be used when either Arm state or Thumb2 is supported.

Would it be better to say:

# builtin support for Targets that have Arm state or have Thumb2 technology.
set(arm_or_thumb2_base_SOURCES
...
418

Suggest:
builtin support for Thumb-only targets without Thumb2 technology, such as v6-m and v8-m.baseline.

508

Suggest base functionality for Arm Targets prior to Arm v7-a and Armv6-m such as v6, v5t, v4t.

nickdesaulniers accepted this revision.Nov 29 2022, 11:29 AM
This revision is now accepted and ready to land.Nov 29 2022, 11:29 AM
stuij updated this revision to Diff 479298.Dec 1 2022, 7:53 AM
stuij marked 3 inline comments as done.

change wording/var as per review comments

stuij added inline comments.Dec 1 2022, 7:55 AM
compiler-rt/lib/builtins/CMakeLists.txt
418

I don't want to come across as too pedantic, but the Arm ARM actually states that v6-m and v8-m do include Thumb2, be it very minor. So I changed the wording slightly. I think/hope the updated wording captures that special class that v6-m and v8-m.baseline occupy.

peter.smith accepted this revision.Dec 1 2022, 8:21 AM

Thanks for the updates LGTM too.

MaskRay accepted this revision.Dec 1 2022, 1:17 PM
MaskRay added inline comments.
compiler-rt/lib/builtins/CMakeLists.txt
378

Targets => targets ?

This revision was automatically updated to reflect the committed changes.