[PATCH] D34682: [Triple] Add isThumb and isARM functions NFCI.

Renato Golin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 11 06:06:29 PDT 2017


rengolin added inline comments.


================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:480
+  bool isThumb = TT.isThumb() ||
                  TT.getSubArch() == Triple::ARMSubArch_v7m ||
                  TT.getSubArch() == Triple::ARMSubArch_v6m;
----------------
fhahn wrote:
> rengolin wrote:
> > javed.absar wrote:
> > > fhahn wrote:
> > > > 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.
> > > Yes I think isMClass would suffice. 
> > 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.
> > 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.
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?


https://reviews.llvm.org/D34682





More information about the llvm-commits mailing list