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

[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:
> 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.


https://reviews.llvm.org/D34682