Page MenuHomePhabricator

[builtins][ARM] Select correct code fragments when compiling for Thumb1/Thum2/ARM ISA
ClosedPublic

Authored by weimingz on Mar 21 2017, 4:10 PM.

Details

Summary

Value of __ARM_ARCH_ISA_THUMB isn't based on the actual compilation
mode (-mthumb, -marm), it reflect's capability of given CPU.

Due to this:
•use tbumb and thumb2 insteand of __ARM_ARCH_ISA_THUMB
•use '.thumb' directive consistently in all affected files
•decorate all thumb functions using DEFINE_COMPILERRT_THUMB_FUNCTION()

(This is based off Michal's patch https://reviews.llvm.org/D30938)

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz created this revision.Mar 21 2017, 4:10 PM
compnerd requested changes to this revision.Mar 22 2017, 11:54 AM

Some of the implementations are not going to build in thumb1 mode, but would in thumb2, which is why we didnt do this uniformly. I think we need to preserve that, or rewrite the functions.

lib/builtins/assembly.h
105 ↗(On Diff #92566)

Is there a reason to use this alternate macro rather than directly using __thumb2__ and __thumb__ and ordering the sources to prefer thumb2, thumb, arm?

This revision now requires changes to proceed.Mar 22 2017, 11:54 AM
weimingz added inline comments.Mar 22 2017, 12:36 PM
lib/builtins/assembly.h
105 ↗(On Diff #92566)

Sort of. For example, to check if it is Thumb1 only, we need to do
#if defined(thumb) && !defined(thumb2)
To check if it's on ARM mode, we need to do
#if defined(
arm) && !defined(thumb) && !defined(thumb2__)

So still nice to have an indirect macro.

compnerd added inline comments.Mar 24 2017, 10:20 AM
lib/builtins/assembly.h
105 ↗(On Diff #92566)

Or we can do the following:

#if defined(__thumb2__)
  ...
#elif defined(__thumb__)
  ...
#else
  ...
#endif

The idea being that thumb2 is preferred, thumb1 accepted, and we fall back to ARM otherwise. Is there something which cannot be represented that way?

weimingz added inline comments.Mar 24 2017, 10:27 AM
lib/builtins/assembly.h
105 ↗(On Diff #92566)

A lot of code work for both thumb2 and arm under the unified syntax.
What we need is to specialize for thumb1.

strejda added inline comments.Mar 27 2017, 10:23 PM
lib/builtins/assembly.h
105 ↗(On Diff #92566)

"The idea being that thumb2 is preferred,"

Preferred? I'm afraid that we have not choice.
Thumb[1][2] must be supported by OS, ARM ISA isn't available on all CPUs. Thus so we cannot hard-force ISA for any file...

Also interworking is slightly problematic, it needs compiler/linker support, and is slow in many cases.
By this all, I don't see any other "realistic" option that compile whole library using one ISA.

@weimingz, what's the current status of this patch? Currently, AFAIK, there is one place where incorrect code is generated without these fixes because it depends on the size of the code generated, that is BLOCK_SIZE in udivsi3.S when compiled with -march=armv6 -marm (see https://github.com/rust-lang-nursery/compiler-builtins/issues/173).

weimingz updated this revision to Diff 106478.Jul 13 2017, 11:18 AM
weimingz edited edge metadata.

rebased

jamesduley added inline comments.Jul 14 2017, 1:46 AM
lib/builtins/assembly.h
74 ↗(On Diff #106478)

@weimingz thanks for the rebase, just one more thing I've noticed, I believe this can also be !defined(USE_THUMB_1).

Just a few comments on the ARM specific terminology. I think it would be better to match the existing documentation such as using state for ARM/Thumb and not mode.

lib/builtins/assembly.h
102 ↗(On Diff #106478)

I think DEFINE_CODE_STATE would be a better choice of name. This would match the ARM documentation more closely. ARM and Thumb are instruction states, this is distinct from modes which are User, IRQ, FIQ etc.

110 ↗(On Diff #106478)

I think it would be better to say USE_THUMB rather than USE_THUMB_1.

Strictly speaking there isn't such a thing as Thumb 1. There is just ARM and Thumb states and Thumb 2 is a set of extensions called "Thumb 2 technology".

test/asan/CMakeLists.txt
10 ↗(On Diff #106478)

I'm in no position to judge whether removing this is the right thing to do, but I can't immediately see how it is related to this patch?

weimingz updated this revision to Diff 106677.Jul 14 2017, 11:28 AM

address the review comments by @peter.smith and @jamesduley

I'm happy with the changes, probably best to wait for compnerd to approve though.

Although not for this patch. On older pre cortex cores such as the ARM1176JF-S (used in the original raspberry pi) that have Thumb but not Thumb2, and crucially not the v6m system extensions, it may be better to switch to ARM state for performance critical functions. Alternatively compiler-rt could always be compiled entirely in ARM state.

compnerd accepted this revision.Aug 12 2017, 10:06 PM

Please adjust the comment and remove the CMake change prior to committing this.

lib/builtins/assembly.h
90 ↗(On Diff #106677)

Use:

// defined(__thumb2__)
test/asan/CMakeLists.txt
10 ↗(On Diff #106478)

This shouldn't be part of this patch.

This revision is now accepted and ready to land.Aug 12 2017, 10:06 PM
weimingz updated this revision to Diff 111060.Aug 14 2017, 1:48 PM

remove unreleated change (CMakefile) and address comments from Saleem

This revision was automatically updated to reflect the committed changes.