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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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;
----------------
rengolin wrote:
> 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?
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?


https://reviews.llvm.org/D34682