[Triple] Add isThumb and isARM functions.
ClosedPublic

Authored by fhahn on Jun 27 2017, 4:57 AM.

Details

Summary

isThumb returns true for Thumb triples (little and big endian), isARM
returns true for ARM triples (little and big endian).
There are a few more checks using arm/thumb that are not covered by
those functions, e.g. that the architecture is either ARM or Thumb
(little endian) or ARM/Thumb little endian only.

Diff Detail

fhahn created this revision.Jun 27 2017, 4:57 AM
javed.absar added inline comments.Jun 28 2017, 6:06 AM
include/llvm/ADT/Triple.h
642

Would it be going too far to have isARMOrThumb defined as well while we are at it.
I see use cases of it.

fhahn updated this revision to Diff 108444.Jul 27 2017, 4:07 AM

rebased, after a long time of inactivity.

fhahn added inline comments.Jul 27 2017, 4:08 AM
include/llvm/ADT/Triple.h
642

I am not sure if that's worth it. Using TT.isARM() || TT.isThumb() is just as readable IMO.

lib/Target/ARM/ARMAsmPrinter.cpp
478–479

I am slightly worried about those checks here. Should isThumb() return true for all M profile CPUs as they only support thumb-mode? Also, it seems that v8m is missing from this check.

javed.absar added inline comments.Jul 28 2017, 10:25 AM
include/llvm/ADT/Triple.h
642

agree

lib/Target/ARM/ARMAsmPrinter.cpp
478–479

Yes I think isMClass would suffice.

rengolin added inline comments.Jul 28 2017, 3:22 PM
lib/Target/ARM/ARMAsmPrinter.cpp
478–479

There are other patches that are taking care of not allowing ARM+M-class, but there is nothing in Triple that explicitly forbids it, and people can still use directives and attributes to have files emit ARM code in an M-class object file with the guarantee that it will never be executed.

I think you can only make M-class == Thumb by default if you guarantee when creating the Triple class that it'll be impossible to create an ARM+M Triple, which in turn will make the M-class check redundant.

I'm curious as to what kind of problem was "fixed" by the additional check and how can we fix it properly instead.

fhahn added inline comments.Jul 31 2017, 7:19 AM
lib/Target/ARM/ARMAsmPrinter.cpp
478–479

There are other patches that are taking care of not allowing ARM+M-class, but there is nothing in Triple that explicitly forbids it, and people can still use directives and attributes to have files emit ARM code in an M-class object file with the guarantee that it will never be executed.

I think you can only make M-class == Thumb by default if you guarantee when creating the Triple class that it'll be impossible to create an ARM+M Triple, which in turn will make the M-class check redundant.

I'm curious as to what kind of problem was "fixed" by the additional check and how can we fix it properly instead.

I think the problem addressed with this check is that we need to know the "default" thumbness of the module to emit the correct .code assembler attribute at the beginning of the file. We cannot get a instance of ARMSubtarget at this point to query the thumbness, because they are per-function. If there would be a way to get a ARMSubtarget instance, that would be ideal....

The Clang toolchain driver currently sets the archname to thumb, for M-class architectures, so I think checking for archname should be enough to determine thumbness at the moment. With D35627, we would generate an error in ARMSubtarget, if the ARM mode is selected for a Thumb-only architecture.

rengolin added inline comments.Aug 11 2017, 6:06 AM
lib/Target/ARM/ARMAsmPrinter.cpp
478–479

I remember a discussion on this a few years ago. Getting any one function would mean lottery coding, which isn't stable. The triple is the only sure way to get a default, because that's what we use to set the other build attributes, even if each function sets their own .code directives.

The M-class problem isn't unique. Old cores (ex. ARMv4) don't support Thumb at all, while CortexM doesn't support ARM. Unfortunately, nothing guarantees any ARM-licensed core will support all of the base standard, so guessing it from the family may not be enough.

Practically speaking, libraries built for M-class can still contain ARM code, if they're also used in non-M-class cores (for example, a single library built for both 6T2 and 6M), with the guarantee that T2 code won't be used in M-class cores. This is not unheard of.

So, my point is that we shouldn't assume "M-class means Thumb", but we should make "M class set thumb", and then just use isThumb. This also simplifies the rest of the code (there are a lot of other cases where we do similar checks on "thumb or M" elsewhere).

Makes sense?

fhahn updated this revision to Diff 110836.Aug 12 2017, 7:25 AM
fhahn retitled this revision from [Triple] Add isThumb and isARM functions NFCI. to [Triple] Add isThumb and isARM functions..

Removed M-class triple check.

Thanks @rengolin for your detailed response. I agree that taking a random function is not a good idea and we have to stick with checking the triple.

lib/Target/ARM/ARMAsmPrinter.cpp
478–479

Yes thanks for sharing the insight. I went ahead and updated the code to just use isThumb(). I'll go over the codebase and will try to get rid of M-class checks if possible in a separate patch, if that's OK?

rengolin accepted this revision.Aug 12 2017, 8:24 AM

Yup. LGTM. Thanks!

This revision is now accepted and ready to land.Aug 12 2017, 8:24 AM
fhahn closed this revision.Aug 12 2017, 10:41 AM