[llvm-commits] [llvm] r139125 - in /llvm/trunk/lib/Target: ARM/ARM.td ARM/ARMInstrInfo.td ARM/ARMSubtarget.cpp ARM/ARMSubtarget.h ARM/MCTargetDesc/ARMMCTargetDesc.cpp X86/MCTargetDesc/X86MCTargetDesc.cpp X86/X86.td X86/X86InstrInfo.td X86/X86Subt

[Jim Grosbach]

On Sep 27, 2011, at 6:36 PM, David Meyer wrote:

> Hello,
> 
> I'm sorry, I did not see this thread earlier.
> 
> The original reason for adding "NaClMode" was because we needed an AssemblerPredicate so that we could disable some MnemonicAlias's in the assembler for NaCl.
> 
> Because NaCl on X86-64 implies 32-bit pointers, certain aliases no longer make sense. (like call -> callq)
> 
> In retrospect, this is not terribly important. I think we can live without it.
> 
> Attached is a patch to remove it. Let me know if it's ok to commit.
> 
> - pdox

Hi David,

This is much closer, thank you. A few comments. OK to commit with the below changes.

> Index: lib/Target/ARM/ARMSubtarget.cpp
> ===================================================================
> --- lib/Target/ARM/ARMSubtarget.cpp	(revision 140669)
> +++ lib/Target/ARM/ARMSubtarget.cpp	(working copy)
> @@ -53,7 +53,6 @@
>    , HasVMLxForwarding(false)
>    , SlowFPBrcc(false)
>    , InThumbMode(false)
> -  , InNaClMode(false)
>    , HasThumb2(false)
>    , NoARM(false)
>    , PostRAScheduler(false)
> Index: lib/Target/ARM/ARMInstrInfo.td
> ===================================================================
> --- lib/Target/ARM/ARMInstrInfo.td	(revision 140669)
> +++ lib/Target/ARM/ARMInstrInfo.td	(working copy)
> @@ -209,8 +209,7 @@
>                                   AssemblerPredicate<"!ModeThumb">;
>  def IsDarwin         : Predicate<"Subtarget->isTargetDarwin()">;
>  def IsNotDarwin      : Predicate<"!Subtarget->isTargetDarwin()">;
> -def IsNaCl           : Predicate<"Subtarget->isTargetNaCl()">,
> -                                 AssemblerPredicate<"ModeNaCl">;
> +def IsNaCl           : Predicate<"Subtarget->isTargetNaCl()">;
>  
>  // FIXME: Eventually this will be just "hasV6T2Ops".
>  def UseMovt          : Predicate<"Subtarget->useMovt()">;
> Index: lib/Target/ARM/ARMSubtarget.h
> ===================================================================
> --- lib/Target/ARM/ARMSubtarget.h	(revision 140669)
> +++ lib/Target/ARM/ARMSubtarget.h	(working copy)
> @@ -70,9 +70,6 @@
>    /// InThumbMode - True if compiling for Thumb, false for ARM.
>    bool InThumbMode;
>  
> -  /// InNaClMode - True if targeting Native Client
> -  bool InNaClMode;
> -
>    /// HasThumb2 - True if Thumb2 instructions are supported.
>    bool HasThumb2;
>  
> Index: lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
> ===================================================================
> --- lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp	(revision 140669)
> +++ lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp	(working copy)
> @@ -86,14 +86,6 @@
>        ARMArchFeature += ",+thumb-mode";
>    }
>  
> -  Triple TheTriple(TT);
> -  if (TheTriple.getOS() == Triple::NativeClient) {
> -    if (ARMArchFeature.empty())
> -      ARMArchFeature = "+nacl-mode";
> -    else
> -      ARMArchFeature += ",+nacl-mode";
> -  }
> -
>    return ARMArchFeature;
>  }
>  
> Index: lib/Target/ARM/ARM.td
> ===================================================================
> --- lib/Target/ARM/ARM.td	(revision 140669)
> +++ lib/Target/ARM/ARM.td	(working copy)
> @@ -23,9 +23,6 @@
>  def ModeThumb  : SubtargetFeature<"thumb-mode", "InThumbMode", "true",
>                                    "Thumb mode">;
>  
> -def ModeNaCl   : SubtargetFeature<"nacl-mode", "InNaClMode", "true",
> -                                  "Native client mode">;
> -
>  //===----------------------------------------------------------------------===//
>  // ARM Subtarget features.
>  //
> Index: lib/Target/X86/X86InstrInfo.td
> ===================================================================
> --- lib/Target/X86/X86InstrInfo.td	(revision 140669)
> +++ lib/Target/X86/X86InstrInfo.td	(working copy)
> @@ -482,14 +482,10 @@
>                               AssemblerPredicate<"Mode64Bit">;
>  def IsWin64      : Predicate<"Subtarget->isTargetWin64()">;
>  def NotWin64     : Predicate<"!Subtarget->isTargetWin64()">;
> -def IsNaCl       : Predicate<"Subtarget->isTargetNaCl()">,
> -                             AssemblerPredicate<"ModeNaCl">;
> -def IsNaCl32     : Predicate<"Subtarget->isTargetNaCl32()">,
> -                             AssemblerPredicate<"ModeNaCl,!Mode64Bit">;
> -def IsNaCl64     : Predicate<"Subtarget->isTargetNaCl64()">,
> -                             AssemblerPredicate<"ModeNaCl,Mode64Bit">;
> -def NotNaCl      : Predicate<"!Subtarget->isTargetNaCl()">,
> -                             AssemblerPredicate<"!ModeNaCl">;
> +def IsNaCl       : Predicate<"Subtarget->isTargetNaCl()">;
> +def IsNaCl32     : Predicate<"Subtarget->isTargetNaCl32()">;
> +def IsNaCl64     : Predicate<"Subtarget->isTargetNaCl64()">;

There shouldn't be a need to have the 32 and 64 versions here. You can use both IsNaCl and Is64Bit in a pattern predicate, for example, to get the same effect. For example,"Requires<[In64BitMode, IsNaCl]>".

> +def NotNaCl      : Predicate<"!Subtarget->isTargetNaCl()">;

It strikes me as odd that both the positive and negative variant is needed, but I see there's precedent for that. No biggie, just a bit of a "huh. Tablegen should be expressive enough that's not necessary," observation.

>  def SmallCode    : Predicate<"TM.getCodeModel() == CodeModel::Small">;
>  def KernelCode   : Predicate<"TM.getCodeModel() == CodeModel::Kernel">;
>  def FarData      : Predicate<"TM.getCodeModel() != CodeModel::Small &&"
> Index: lib/Target/X86/X86.td
> ===================================================================
> --- lib/Target/X86/X86.td	(revision 140669)
> +++ lib/Target/X86/X86.td	(working copy)
> @@ -23,9 +23,6 @@
>  def Mode64Bit : SubtargetFeature<"64bit-mode", "In64BitMode", "true",
>                                    "64-bit mode (x86_64)">;
>  
> -def ModeNaCl  : SubtargetFeature<"nacl-mode", "InNaClMode", "true",
> -                                 "Native Client mode">;
> -
>  //===----------------------------------------------------------------------===//
>  // X86 Subtarget features.
>  //===----------------------------------------------------------------------===//
> Index: lib/Target/X86/X86Subtarget.cpp
> ===================================================================
> --- lib/Target/X86/X86Subtarget.cpp	(revision 140669)
> +++ lib/Target/X86/X86Subtarget.cpp	(working copy)
> @@ -262,8 +262,7 @@
>    // FIXME: this is a known good value for Yonah. How about others?
>    , MaxInlineSizeThreshold(128)
>    , TargetTriple(TT)
> -  , In64BitMode(is64Bit)
> -  , InNaClMode(false) {
> +  , In64BitMode(is64Bit) {
>    // Determine default and user specified characteristics
>    if (!FS.empty() || !CPU.empty()) {
>      std::string CPUName = CPU;
> @@ -309,11 +308,6 @@
>    if (In64BitMode)
>      ToggleFeature(X86::Mode64Bit);
>  
> -  if (isTargetNaCl()) {
> -    InNaClMode = true;
> -    ToggleFeature(X86::ModeNaCl);
> -  }
> -
>    if (HasAVX)
>      X86SSELevel = NoMMXSSE;
>      
> Index: lib/Target/X86/X86Subtarget.h
> ===================================================================
> --- lib/Target/X86/X86Subtarget.h	(revision 140669)
> +++ lib/Target/X86/X86Subtarget.h	(working copy)
> @@ -119,9 +119,6 @@
>    /// In64BitMode - True if compiling for 64-bit, false for 32-bit.
>    bool In64BitMode;
>  
> -  /// InNaClMode - True if compiling for Native Client target.
> -  bool InNaClMode;
> -
>  public:
>  
>    /// This constructor initializes the data members to match that
> Index: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
> ===================================================================
> --- lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp	(revision 140669)
> +++ lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp	(working copy)
> @@ -45,10 +45,6 @@
>      FS = "+64bit-mode";
>    else
>      FS = "-64bit-mode";
> -  if (TheTriple.getOS() == Triple::NativeClient)
> -    FS += ",+nacl-mode";
> -  else
> -    FS += ",-nacl-mode";
>    return FS;
>  }
>  
>