[Triple] Add isThumb and isARM functions.

[fhahn]

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.

[javed.absar]

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]

rebased, after a long time of inactivity.

[fhahn]

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]

include/llvm/ADT/Triple.h

642

agree

lib/Target/ARM/ARMAsmPrinter.cpp

478–479

Yes I think isMClass would suffice.

[rengolin]

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]

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]

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]

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]

Yup. LGTM. Thanks!

include/llvm/ADT/Triple.h

return getEnvironment() == Triple::Musl ||
            getEnvironment() == Triple::MuslEABIHF;
   }
 
   /// Tests whether the target is NVPTX (32- or 64-bit).
   bool isNVPTX() const {
     return getArch() == Triple::nvptx || getArch() == Triple::nvptx64;
   }
 
+  /// Tests whether the target is Thumb (little and big endian).
+  bool isThumb() const {
+    return getArch() == Triple::thumb || getArch() == Triple::thumbeb;
+  }
+
+  /// Tests whether the target is ARM (little and big endian).
+  bool isARM() const {
+    return getArch() == Triple::arm || getArch() == Triple::armeb;
+  }
+
   /// Tests wether the target supports comdat
   bool supportsCOMDAT() const { return !isOSBinFormatMachO(); }
 
   /// @}
   /// @name Mutators
   /// @{
 
   /// setArch - Set the architecture (first) component of the triple

lib/MC/MCSectionELF.cpp

void MCSectionELF::PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
 
   // If there are target-specific flags, print them.
   Triple::ArchType Arch = T.getArch();
   if (Arch == Triple::xcore) {
     if (Flags & ELF::XCORE_SHF_CP_SECTION)
       OS << 'c';
     if (Flags & ELF::XCORE_SHF_DP_SECTION)
       OS << 'd';
-  } else if (Arch == Triple::arm || Arch == Triple::armeb ||
-             Arch == Triple::thumb || Arch == Triple::thumbeb) {
+  } else if (T.isARM() || T.isThumb()) {
     if (Flags & ELF::SHF_ARM_PURECODE)
       OS << 'y';
   }
 
   OS << '"';
 
   OS << ',';
 

lib/Object/ELFObjectFile.cpp

void ELFObjectFileBase::setARMSubArch(Triple &TheTriple) const {
 
   ARMAttributeParser Attributes;
   std::error_code EC = getBuildAttributes(Attributes);
   if (EC)
     return;
 
   std::string Triple;
   // Default to ARM, but use the triple if it's been set.
-  if (TheTriple.getArch() == Triple::thumb ||
-      TheTriple.getArch() == Triple::thumbeb)
+  if (TheTriple.isThumb())
     Triple = "thumb";
   else
     Triple = "arm";
 
   if (Attributes.hasAttribute(ARMBuildAttrs::CPU_arch)) {
     switch(Attributes.getAttributeValue(ARMBuildAttrs::CPU_arch)) {
     case ARMBuildAttrs::v4:
       Triple += "v4";

lib/Target/ARM/ARMAsmPrinter.h

bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
 public:
   unsigned getISAEncoding() override {
     // ARM/Darwin adds ISA to the DWARF info for each function.
     const Triple &TT = TM.getTargetTriple();
     if (!TT.isOSBinFormatMachO())
       return 0;
-    bool isThumb = TT.getArch() == Triple::thumb ||
-                   TT.getArch() == Triple::thumbeb ||
+    bool isThumb = TT.isThumb() ||
                    TT.getSubArch() == Triple::ARMSubArch_v7m ||
                    TT.getSubArch() == Triple::ARMSubArch_v6m;
     return isThumb ? ARM::DW_ISA_ARM_thumb : ARM::DW_ISA_ARM_arm;
   }
 
 private:
   MCOperand GetSymbolRef(const MachineOperand &MO, const MCSymbol *Symbol);
   MCSymbol *GetARMJTIPICJumpTableLabel(unsigned uid) const;

lib/Target/ARM/ARMAsmPrinter.cpp

void ARMAsmPrinter::EmitStartOfAsmFile(Module &M) {
   OutStreamer->EmitAssemblerFlag(MCAF_SyntaxUnified);
 
   // Emit ARM Build Attributes
   if (TT.isOSBinFormatELF())
     emitAttributes();
 
   // Use the triple's architecture and subarchitecture to determine
   // if we're thumb for the purposes of the top level code16 assembler
   // flag.
-  bool isThumb = TT.getArch() == Triple::thumb ||
-                 TT.getArch() == Triple::thumbeb ||
-                 TT.getSubArch() == Triple::ARMSubArch_v7m ||
-                 TT.getSubArch() == Triple::ARMSubArch_v6m;
-  if (!M.getModuleInlineAsm().empty() && isThumb)
+  if (!M.getModuleInlineAsm().empty() && TT.isThumb())
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
 }
 
 static void
 emitNonLazySymbolPointer(MCStreamer &OutStreamer, MCSymbol *StubLabel,
                          MachineModuleInfoImpl::StubValueTy &MCSym) {
   // L_foo$stub:
   OutStreamer.EmitLabel(StubLabel);

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

 
 #define GET_INSTRINFO_MC_DESC
 #include "ARMGenInstrInfo.inc"
 
 #define GET_SUBTARGETINFO_MC_DESC
 #include "ARMGenSubtargetInfo.inc"
 
 std::string ARM_MC::ParseARMTriple(const Triple &TT, StringRef CPU) {
-  bool isThumb =
-      TT.getArch() == Triple::thumb || TT.getArch() == Triple::thumbeb;
-
   std::string ARMArchFeature;
 
   ARM::ArchKind ArchID = ARM::parseArch(TT.getArchName());
   if (ArchID != ARM::ArchKind::INVALID &&  (CPU.empty() || CPU == "generic"))
     ARMArchFeature = (ARMArchFeature + "+" + ARM::getArchName(ArchID)).str();
 
-  if (isThumb) {
+  if (TT.isThumb()) {
     if (ARMArchFeature.empty())
       ARMArchFeature = "+thumb-mode";
     else
       ARMArchFeature += ",+thumb-mode";
   }
 
   if (TT.isOSNaCl()) {
     if (ARMArchFeature.empty())