Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[X86] Support EGPR (R16-R31) for APX #67702

Merged
merged 4 commits into from
Oct 10, 2023
Merged

[X86] Support EGPR (R16-R31) for APX #67702

merged 4 commits into from
Oct 10, 2023

Conversation

KanRobert
Copy link
Contributor

@KanRobert KanRobert commented Sep 28, 2023

  1. Map R16-R31 to DWARF registers 130-145.
  2. Make R16-R31 caller-saved registers.
  3. Make R16-31 allocatable only when feature EGPR is supported
  4. Make R16-31 availabe for instructions in legacy maps 0/1 and EVEX space, except XSAVE*/XRSTOR

RFC: https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4

Explanations for some seemingly unrelated changes:

inline-asm-registers.mir, statepoint-invoke-ra-enter-at-end.mir:
The immediate (TargetInstrInfo.cpp:1612) used for the regdef/reguse is the encoding for the register
class in the enum generated by tablegen. This encoding will change
any time a new register class is added. Since the number is part
of the input, this means it can become stale.

seh-directive-errors.s:
R16-R31 makes ".seh_pushreg 17" legal

musttail-varargs.ll:
It seems some LLVM passes use the number of registers rather the number of allocatable registers as heuristic.

RFC: https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4

1. Map R16-R31 to DWARF registers 130-145.
2. Make R16-R31 caller-saved registers.
3. Make R16-31 allocatable only when feature EGPR is supported
4. Make R16-31 availabe for instructions legacy maps 0/1 and EVEX space, except XSAVE*/XRSTOR

Explanations for some seemingly unrelated changes:
inline-asm-registers.mir, statepoint-invoke-ra-enter-at-end.mir:
  The immediate (TargetInstrInfo.cpp:1612) used for the regdef/reguse is the encoding for the register
  class in the enum generated by tablegen. This encoding will change
  any time a new register class is added. Since the number is part
  of the input, this means it can become stale.

seh-directive-errors.s:
   R16-R31 makes ".seh_pushreg 17" legal

musttail-varargs.ll:
  It seems some LLVM passes use the number of registers rather the number of allocatable registers as heuristic.
@llvmbot llvmbot added backend:X86 mc Machine (object) code labels Sep 28, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2023

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-mc

Changes

RFC: https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4

  1. Map R16-R31 to DWARF registers 130-145.
  2. Make R16-R31 caller-saved registers.
  3. Make R16-31 allocatable only when feature EGPR is supported
  4. Make R16-31 availabe for instructions legacy maps 0/1 and EVEX space, except XSAVE*/XRSTOR

Explanations for some seemingly unrelated changes:
inline-asm-registers.mir, statepoint-invoke-ra-enter-at-end.mir:
The immediate (TargetInstrInfo.cpp:1612) used for the regdef/reguse is the encoding for the register
class in the enum generated by tablegen. This encoding will change
any time a new register class is added. Since the number is part
of the input, this means it can become stale.

seh-directive-errors.s:
R16-R31 makes ".seh_pushreg 17" legal

musttail-varargs.ll:
It seems some LLVM passes use the number of registers rather the number of allocatable registers as heuristic.


Patch is 42.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67702.diff

16 Files Affected:

  • (modified) llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h (+38)
  • (modified) llvm/lib/Target/X86/X86.td (+2)
  • (modified) llvm/lib/Target/X86/X86InstrInfo.cpp (+31)
  • (modified) llvm/lib/Target/X86/X86InstrInfo.h (+5)
  • (modified) llvm/lib/Target/X86/X86RegisterInfo.cpp (+12)
  • (modified) llvm/lib/Target/X86/X86RegisterInfo.td (+176-13)
  • (modified) llvm/test/CodeGen/MIR/X86/inline-asm-registers.mir (+4-4)
  • (added) llvm/test/CodeGen/X86/apx/no-rex2-general.ll (+81)
  • (added) llvm/test/CodeGen/X86/apx/no-rex2-pseudo-amx.ll (+18)
  • (added) llvm/test/CodeGen/X86/apx/no-rex2-pseudo-x87.ll (+18)
  • (added) llvm/test/CodeGen/X86/apx/no-rex2-special.ll (+70)
  • (modified) llvm/test/CodeGen/X86/ipra-reg-usage.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/musttail-varargs.ll (+24-24)
  • (modified) llvm/test/CodeGen/X86/statepoint-invoke-ra-enter-at-end.mir (+2-2)
  • (modified) llvm/test/MC/AsmParser/seh-directive-errors.s (+1-1)
  • (added) llvm/test/MC/X86/apx/cfi-reg.s (+41)
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h b/llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
index e2293fe30561fb4..4c5b49fcc1bcd9b 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
@@ -1208,6 +1208,44 @@ namespace X86II {
     return false;
   }
 
+  inline bool canUseApxExtendedReg(const MCInstrDesc &Desc) {
+    uint64_t TSFlags = Desc.TSFlags;
+    uint64_t Encoding = TSFlags & EncodingMask;
+    // EVEX can always use egpr.
+    if (Encoding == X86II::EVEX)
+      return true;
+
+    // MAP OB/TB in legacy encoding space can always use egpr except
+    // XSAVE*/XRSTOR*.
+    unsigned Opcode = Desc.Opcode;
+    bool IsSpecial = false;
+    switch (Opcode) {
+    default:
+      // To be conservative, egpr is not used for all pseudo instructions
+      // because we are not sure what instruction it will become.
+      // FIXME: Could we improve it in X86ExpandPseudo?
+      IsSpecial = isPseudo(TSFlags);
+      break;
+    case X86::XSAVE:
+    case X86::XSAVE64:
+    case X86::XSAVEOPT:
+    case X86::XSAVEOPT64:
+    case X86::XSAVEC:
+    case X86::XSAVEC64:
+    case X86::XSAVES:
+    case X86::XSAVES64:
+    case X86::XRSTOR:
+    case X86::XRSTOR64:
+    case X86::XRSTORS:
+    case X86::XRSTORS64:
+      IsSpecial = true;
+      break;
+    }
+    uint64_t OpMap = TSFlags & X86II::OpMapMask;
+    return !Encoding && (OpMap == X86II::OB || OpMap == X86II::TB) &&
+           !IsSpecial;
+  }
+
   /// \returns true if the MemoryOperand is a 32 extended (zmm16 or higher)
   /// registers, e.g. zmm21, etc.
   static inline bool is32ExtendedReg(unsigned RegNo) {
diff --git a/llvm/lib/Target/X86/X86.td b/llvm/lib/Target/X86/X86.td
index 64f91ae90e2b0ce..6c5fb49ff9f9504 100644
--- a/llvm/lib/Target/X86/X86.td
+++ b/llvm/lib/Target/X86/X86.td
@@ -331,6 +331,8 @@ def FeatureMOVDIRI  : SubtargetFeature<"movdiri", "HasMOVDIRI", "true",
                                        "Support movdiri instruction (direct store integer)">;
 def FeatureMOVDIR64B : SubtargetFeature<"movdir64b", "HasMOVDIR64B", "true",
                                         "Support movdir64b instruction (direct store 64 bytes)">;
+def FeatureEGPR : SubtargetFeature<"egpr", "HasEGPR", "true",
+                                   "Support extended general purpose register">;
 
 // Ivy Bridge and newer processors have enhanced REP MOVSB and STOSB (aka
 // "string operations"). See "REP String Enhancement" in the Intel Software
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 4320a0e94b7a71f..6d1da855761cdd5 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -92,6 +92,37 @@ X86InstrInfo::X86InstrInfo(X86Subtarget &STI)
       Subtarget(STI), RI(STI.getTargetTriple()) {
 }
 
+const TargetRegisterClass *
+X86InstrInfo::getRegClass(const MCInstrDesc &MCID, unsigned OpNum,
+                          const TargetRegisterInfo *TRI,
+                          const MachineFunction &MF) const {
+  auto *RC = TargetInstrInfo::getRegClass(MCID, OpNum, TRI, MF);
+  // If the target does not have egpr, then r16-r31 will be resereved for all
+  // instructions.
+  if (!RC || !Subtarget.hasEGPR())
+    return RC;
+
+  if (X86II::canUseApxExtendedReg(MCID))
+    return RC;
+
+  switch (RC->getID()) {
+  default:
+    return RC;
+  case X86::GR8RegClassID:
+    return &X86::GR8_NOREX2RegClass;
+  case X86::GR16RegClassID:
+    return &X86::GR16_NOREX2RegClass;
+  case X86::GR32RegClassID:
+    return &X86::GR32_NOREX2RegClass;
+  case X86::GR64RegClassID:
+    return &X86::GR64_NOREX2RegClass;
+  case X86::GR32_NOSPRegClassID:
+    return &X86::GR32_NOREX2_NOSPRegClass;
+  case X86::GR64_NOSPRegClassID:
+    return &X86::GR64_NOREX2_NOSPRegClass;
+  }
+}
+
 bool
 X86InstrInfo::isCoalescableExtInstr(const MachineInstr &MI,
                                     Register &SrcReg, Register &DstReg,
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h b/llvm/lib/Target/X86/X86InstrInfo.h
index 8119302f73e8b36..e106b9df8850ba1 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -150,6 +150,11 @@ class X86InstrInfo final : public X86GenInstrInfo {
 public:
   explicit X86InstrInfo(X86Subtarget &STI);
 
+  const TargetRegisterClass *
+  getRegClass(const MCInstrDesc &MCID, unsigned OpNum,
+              const TargetRegisterInfo *TRI,
+              const MachineFunction &MF) const override;
+
   /// getRegisterInfo - TargetInstrInfo is a superset of MRegister info.  As
   /// such, whenever a client has an instance of instruction info, it should
   /// always be able to get register info as well (through this method).
diff --git a/llvm/lib/Target/X86/X86RegisterInfo.cpp b/llvm/lib/Target/X86/X86RegisterInfo.cpp
index 3504ca2b5743f88..7fcc2a1acfd963d 100644
--- a/llvm/lib/Target/X86/X86RegisterInfo.cpp
+++ b/llvm/lib/Target/X86/X86RegisterInfo.cpp
@@ -158,6 +158,10 @@ X86RegisterInfo::getLargestLegalSuperClass(const TargetRegisterClass *RC,
     case X86::GR16RegClassID:
     case X86::GR32RegClassID:
     case X86::GR64RegClassID:
+    case X86::GR8_NOREX2RegClassID:
+    case X86::GR16_NOREX2RegClassID:
+    case X86::GR32_NOREX2RegClassID:
+    case X86::GR64_NOREX2RegClassID:
     case X86::RFP32RegClassID:
     case X86::RFP64RegClassID:
     case X86::RFP80RegClassID:
@@ -610,6 +614,14 @@ BitVector X86RegisterInfo::getReservedRegs(const MachineFunction &MF) const {
     }
   }
 
+  // Reserve the extended general purpose registers.
+  if (!Is64Bit || !MF.getSubtarget<X86Subtarget>().hasEGPR()) {
+    for (unsigned n = 0; n != 16; ++n) {
+      for (MCRegAliasIterator AI(X86::R16 + n, this, true); AI.isValid(); ++AI)
+        Reserved.set(*AI);
+    }
+  }
+
   assert(checkAllSuperRegsMarked(Reserved,
                                  {X86::SIL, X86::DIL, X86::BPL, X86::SPL,
                                   X86::SIH, X86::DIH, X86::BPH, X86::SPH}));
diff --git a/llvm/lib/Target/X86/X86RegisterInfo.td b/llvm/lib/Target/X86/X86RegisterInfo.td
index 1e6477e658b9d10..fe251b75e9c0c4e 100644
--- a/llvm/lib/Target/X86/X86RegisterInfo.td
+++ b/llvm/lib/Target/X86/X86RegisterInfo.td
@@ -73,6 +73,42 @@ def R12B : X86Reg<"r12b", 12>;
 def R13B : X86Reg<"r13b", 13>;
 def R14B : X86Reg<"r14b", 14>;
 def R15B : X86Reg<"r15b", 15>;
+// RAGreedy prefers to select a cheaper register
+// For x86,
+//   Cost(caller-save reg) < Cost(callee-save reg)
+// b/c callee-save register needs push/pop in prolog/epilog.
+// If both registers are callee-saved or caller-saved,
+//   Cost(short-encoding reg) < Cost(long-encoding reg)
+//
+// To achieve this, we do the following things:
+//   1. Set CostPerUse=1 for registers that need prefix
+//   2. Consider callee-save register is never cheaper than a register w/ cost 1
+//   3. List caller-save register before callee-save regsiter in RegisterClass
+//      or AllocationOrder
+//
+// NOTE:
+//   D133902 stopped assigning register costs for R8-R15, which brought gain
+//   and regression. We don't know if we should assign cost to R16-R31 w/o
+//   performance data.
+// TODO:
+//   Update the comment/cost after tuning.
+// APX only, requires REX2 or EVEX.
+def R16B : X86Reg<"r16b", 16>;
+def R17B : X86Reg<"r17b", 17>;
+def R18B : X86Reg<"r18b", 18>;
+def R19B : X86Reg<"r19b", 19>;
+def R20B : X86Reg<"r20b", 20>;
+def R21B : X86Reg<"r21b", 21>;
+def R22B : X86Reg<"r22b", 22>;
+def R23B : X86Reg<"r23b", 23>;
+def R24B : X86Reg<"r24b", 24>;
+def R25B : X86Reg<"r25b", 25>;
+def R26B : X86Reg<"r26b", 26>;
+def R27B : X86Reg<"r27b", 27>;
+def R28B : X86Reg<"r28b", 28>;
+def R29B : X86Reg<"r29b", 29>;
+def R30B : X86Reg<"r30b", 30>;
+def R31B : X86Reg<"r31b", 31>;
 
 let isArtificial = 1 in {
 // High byte of the low 16 bits of the super-register:
@@ -88,6 +124,22 @@ def R12BH : X86Reg<"", -1>;
 def R13BH : X86Reg<"", -1>;
 def R14BH : X86Reg<"", -1>;
 def R15BH : X86Reg<"", -1>;
+def R16BH : X86Reg<"", -1>;
+def R17BH : X86Reg<"", -1>;
+def R18BH : X86Reg<"", -1>;
+def R19BH : X86Reg<"", -1>;
+def R20BH : X86Reg<"", -1>;
+def R21BH : X86Reg<"", -1>;
+def R22BH : X86Reg<"", -1>;
+def R23BH : X86Reg<"", -1>;
+def R24BH : X86Reg<"", -1>;
+def R25BH : X86Reg<"", -1>;
+def R26BH : X86Reg<"", -1>;
+def R27BH : X86Reg<"", -1>;
+def R28BH : X86Reg<"", -1>;
+def R29BH : X86Reg<"", -1>;
+def R30BH : X86Reg<"", -1>;
+def R31BH : X86Reg<"", -1>;
 // High word of the low 32 bits of the super-register:
 def HAX   : X86Reg<"", -1>;
 def HDX   : X86Reg<"", -1>;
@@ -106,6 +158,22 @@ def R12WH : X86Reg<"", -1>;
 def R13WH : X86Reg<"", -1>;
 def R14WH : X86Reg<"", -1>;
 def R15WH : X86Reg<"", -1>;
+def R16WH : X86Reg<"", -1>;
+def R17WH : X86Reg<"", -1>;
+def R18WH : X86Reg<"", -1>;
+def R19WH : X86Reg<"", -1>;
+def R20WH : X86Reg<"", -1>;
+def R21WH : X86Reg<"", -1>;
+def R22WH : X86Reg<"", -1>;
+def R23WH : X86Reg<"", -1>;
+def R24WH : X86Reg<"", -1>;
+def R25WH : X86Reg<"", -1>;
+def R26WH : X86Reg<"", -1>;
+def R27WH : X86Reg<"", -1>;
+def R28WH : X86Reg<"", -1>;
+def R29WH : X86Reg<"", -1>;
+def R30WH : X86Reg<"", -1>;
+def R31WH : X86Reg<"", -1>;
 }
 
 // 16-bit registers
@@ -134,6 +202,25 @@ def R13W : X86Reg<"r13w", 13, [R13B,R13BH]>;
 def R14W : X86Reg<"r14w", 14, [R14B,R14BH]>;
 def R15W : X86Reg<"r15w", 15, [R15B,R15BH]>;
 }
+// APX only, requires REX2 or EVEX.
+let SubRegIndices = [sub_8bit, sub_8bit_hi_phony], CoveredBySubRegs = 1 in {
+def R16W : X86Reg<"r16w", 16, [R16B,R16BH]>;
+def R17W : X86Reg<"r17w", 17, [R17B,R17BH]>;
+def R18W : X86Reg<"r18w", 18, [R18B,R18BH]>;
+def R19W : X86Reg<"r19w", 19, [R19B,R19BH]>;
+def R20W : X86Reg<"r20w", 20, [R20B,R20BH]>;
+def R21W : X86Reg<"r21w", 21, [R21B,R21BH]>;
+def R22W : X86Reg<"r22w", 22, [R22B,R22BH]>;
+def R23W : X86Reg<"r23w", 23, [R23B,R23BH]>;
+def R24W : X86Reg<"r24w", 24, [R24B,R24BH]>;
+def R25W : X86Reg<"r25w", 25, [R25B,R25BH]>;
+def R26W : X86Reg<"r26w", 26, [R26B,R26BH]>;
+def R27W : X86Reg<"r27w", 27, [R27B,R27BH]>;
+def R28W : X86Reg<"r28w", 28, [R28B,R28BH]>;
+def R29W : X86Reg<"r29w", 29, [R29B,R29BH]>;
+def R30W : X86Reg<"r30w", 30, [R30B,R30BH]>;
+def R31W : X86Reg<"r31w", 31, [R31B,R31BH]>;
+}
 
 // 32-bit registers
 let SubRegIndices = [sub_16bit, sub_16bit_hi], CoveredBySubRegs = 1 in {
@@ -160,6 +247,25 @@ def R14D : X86Reg<"r14d", 14, [R14W,R14WH]>;
 def R15D : X86Reg<"r15d", 15, [R15W,R15WH]>;
 }
 
+// APX only, requires REX2 or EVEX.
+let SubRegIndices = [sub_16bit, sub_16bit_hi], CoveredBySubRegs = 1 in {
+def R16D : X86Reg<"r16d", 16, [R16W,R16WH]>;
+def R17D : X86Reg<"r17d", 17, [R17W,R17WH]>;
+def R18D : X86Reg<"r18d", 18, [R18W,R18WH]>;
+def R19D : X86Reg<"r19d", 19, [R19W,R19WH]>;
+def R20D : X86Reg<"r20d", 20, [R20W,R20WH]>;
+def R21D : X86Reg<"r21d", 21, [R21W,R21WH]>;
+def R22D : X86Reg<"r22d", 22, [R22W,R22WH]>;
+def R23D : X86Reg<"r23d", 23, [R23W,R23WH]>;
+def R24D : X86Reg<"r24d", 24, [R24W,R24WH]>;
+def R25D : X86Reg<"r25d", 25, [R25W,R25WH]>;
+def R26D : X86Reg<"r26d", 26, [R26W,R26WH]>;
+def R27D : X86Reg<"r27d", 27, [R27W,R27WH]>;
+def R28D : X86Reg<"r28d", 28, [R28W,R28WH]>;
+def R29D : X86Reg<"r29d", 29, [R29W,R29WH]>;
+def R30D : X86Reg<"r30d", 30, [R30W,R30WH]>;
+def R31D : X86Reg<"r31d", 31, [R31W,R31WH]>;
+}
 // 64-bit registers, X86-64 only
 let SubRegIndices = [sub_32bit] in {
 def RAX : X86Reg<"rax", 0, [EAX]>, DwarfRegNum<[0, -2, -2]>;
@@ -181,6 +287,23 @@ def R13 : X86Reg<"r13", 13, [R13D]>, DwarfRegNum<[13, -2, -2]>;
 def R14 : X86Reg<"r14", 14, [R14D]>, DwarfRegNum<[14, -2, -2]>;
 def R15 : X86Reg<"r15", 15, [R15D]>, DwarfRegNum<[15, -2, -2]>;
 def RIP : X86Reg<"rip",  0, [EIP]>,  DwarfRegNum<[16, -2, -2]>;
+// APX only, requires REX2 or EVEX.
+def R16 : X86Reg<"r16", 16, [R16D]>, DwarfRegNum<[130, -2, -2]>;
+def R17 : X86Reg<"r17", 17, [R17D]>, DwarfRegNum<[131, -2, -2]>;
+def R18 : X86Reg<"r18", 18, [R18D]>, DwarfRegNum<[132, -2, -2]>;
+def R19 : X86Reg<"r19", 19, [R19D]>, DwarfRegNum<[133, -2, -2]>;
+def R20 : X86Reg<"r20", 20, [R20D]>, DwarfRegNum<[134, -2, -2]>;
+def R21 : X86Reg<"r21", 21, [R21D]>, DwarfRegNum<[135, -2, -2]>;
+def R22 : X86Reg<"r22", 22, [R22D]>, DwarfRegNum<[136, -2, -2]>;
+def R23 : X86Reg<"r23", 23, [R23D]>, DwarfRegNum<[137, -2, -2]>;
+def R24 : X86Reg<"r24", 24, [R24D]>, DwarfRegNum<[138, -2, -2]>;
+def R25 : X86Reg<"r25", 25, [R25D]>, DwarfRegNum<[139, -2, -2]>;
+def R26 : X86Reg<"r26", 26, [R26D]>, DwarfRegNum<[140, -2, -2]>;
+def R27 : X86Reg<"r27", 27, [R27D]>, DwarfRegNum<[141, -2, -2]>;
+def R28 : X86Reg<"r28", 28, [R28D]>, DwarfRegNum<[142, -2, -2]>;
+def R29 : X86Reg<"r29", 29, [R29D]>, DwarfRegNum<[143, -2, -2]>;
+def R30 : X86Reg<"r30", 30, [R30D]>, DwarfRegNum<[144, -2, -2]>;
+def R31 : X86Reg<"r31", 31, [R31D]>, DwarfRegNum<[145, -2, -2]>;
 }
 
 // MMX Registers. These are actually aliased to ST0 .. ST7
@@ -390,9 +513,11 @@ def SSP : X86Reg<"ssp", 0>;
 // instruction requiring a REX prefix, while SIL, DIL, BPL, R8D, etc.
 // require a REX prefix. For example, "addb %ah, %dil" and "movzbl %ah, %r8d"
 // cannot be encoded.
-def GR8 : RegisterClass<"X86", [i8],  8,
+def GR8 : RegisterClass<"X86", [i8], 8,
                         (add AL, CL, DL, AH, CH, DH, BL, BH, SIL, DIL, BPL, SPL,
-                             R8B, R9B, R10B, R11B, R14B, R15B, R12B, R13B)> {
+                         R8B, R9B, R10B, R11B, R16B, R17B, R18B, R19B, R20B,
+                         R21B, R22B, R23B, R24B, R25B, R26B, R27B, R28B, R29B,
+                         R30B, R31B, R14B, R15B, R12B, R13B)> {
   let AltOrders = [(sub GR8, AH, BH, CH, DH)];
   let AltOrderSelect = [{
     return MF.getSubtarget<X86Subtarget>().is64Bit();
@@ -400,23 +525,30 @@ def GR8 : RegisterClass<"X86", [i8],  8,
 }
 
 let isAllocatable = 0 in
-def GRH8 : RegisterClass<"X86", [i8],  8,
+def GRH8 : RegisterClass<"X86", [i8], 8,
                          (add SIH, DIH, BPH, SPH, R8BH, R9BH, R10BH, R11BH,
-                              R12BH, R13BH, R14BH, R15BH)>;
 
+                          R12BH, R13BH, R14BH, R15BH, R16BH, R17BH, R18BH,
+                          R19BH, R20BH, R21BH, R22BH, R23BH, R24BH, R25BH,
+                          R26BH, R27BH, R28BH, R29BH, R30BH, R31BH)>;
 def GR16 : RegisterClass<"X86", [i16], 16,
-                         (add AX, CX, DX, SI, DI, BX, BP, SP,
-                              R8W, R9W, R10W, R11W, R14W, R15W, R12W, R13W)>;
+                         (add AX, CX, DX, SI, DI, BX, BP, SP, R8W, R9W, R10W,
+                          R11W, R16W, R17W, R18W, R19W, R20W, R21W, R22W, R23W,
+                          R24W, R25W, R26W, R27W, R28W, R29W, R30W, R31W, R14W,
+                          R15W, R12W, R13W)>;
 
 let isAllocatable = 0 in
 def GRH16 : RegisterClass<"X86", [i16], 16,
-                          (add HAX, HCX, HDX, HSI, HDI, HBX, HBP, HSP, HIP,
-                               R8WH, R9WH, R10WH, R11WH, R12WH, R13WH, R14WH,
-                               R15WH)>;
 
+                    (add HAX, HCX, HDX, HSI, HDI, HBX, HBP, HSP, HIP, R8WH,
+                     R9WH, R10WH, R11WH, R12WH, R13WH, R14WH, R15WH, R16WH,
+                     R17WH, R18WH, R19WH, R20WH, R21WH, R22WH, R23WH, R24WH,
+                     R25WH, R26WH, R27WH, R28WH, R29WH, R30WH, R31WH)>;
 def GR32 : RegisterClass<"X86", [i32], 32,
-                         (add EAX, ECX, EDX, ESI, EDI, EBX, EBP, ESP,
-                              R8D, R9D, R10D, R11D, R14D, R15D, R12D, R13D)>;
+                         (add EAX, ECX, EDX, ESI, EDI, EBX, EBP, ESP, R8D, R9D,
+                          R10D, R11D, R16D, R17D, R18D, R19D, R20D, R21D, R22D,
+                          R23D, R24D, R25D, R26D, R27D, R28D, R29D, R30D, R31D,
+                          R14D, R15D, R12D, R13D)>;
 
 // GR64 - 64-bit GPRs. This oddly includes RIP, which isn't accurate, since
 // RIP isn't really a register and it can't be used anywhere except in an
@@ -424,8 +556,9 @@ def GR32 : RegisterClass<"X86", [i32], 32,
 // FIXME: it *does* cause trouble - CheckBaseRegAndIndexReg() has extra
 // tests because of the inclusion of RIP in this register class.
 def GR64 : RegisterClass<"X86", [i64], 64,
-                         (add RAX, RCX, RDX, RSI, RDI, R8, R9, R10, R11,
-                              RBX, R14, R15, R12, R13, RBP, RSP, RIP)>;
+                    (add RAX, RCX, RDX, RSI, RDI, R8, R9, R10, R11, R16, R17,
+                     R18, R19, R20, R21, R22, R23, R24, R25, R26, R27, R28, R29,
+                     R30, R31, RBX, R14, R15, R12, R13, RBP, RSP, RIP)>;
 
 // GR64PLTSafe - 64-bit GPRs without R10, R11, RSP and RIP. Could be used when
 // emitting code for intrinsics, which use implict input registers.
@@ -491,6 +624,27 @@ def GR32_NOREX : RegisterClass<"X86", [i32], 32,
 // GR64_NOREX - GR64 registers which do not require a REX prefix.
 def GR64_NOREX : RegisterClass<"X86", [i64], 64,
                             (add RAX, RCX, RDX, RSI, RDI, RBX, RBP, RSP, RIP)>;
+// GeneratePressureSet = 0 here is a temporary workaround for lots of
+// LIT fail for xisa. Whether enabling in the future still needs discussion.
+let GeneratePressureSet = 0 in {
+// GR8_NOREX2 - GR8 registers which do not require a REX2 prefix.
+def GR8_NOREX2 : RegisterClass<"X86", [i8], 8,
+                               (sub GR8,  (sequence "R%uB", 16, 31))> {
+  let AltOrders = [(sub GR8_NOREX2, AH, BH, CH, DH)];
+  let AltOrderSelect = [{
+    return MF.getSubtarget<X86Subtarget>().is64Bit();
+  }];
+}
+// GR16_NOREX2 - GR16 registers which do not require a REX2 prefix.
+def GR16_NOREX2 : RegisterClass<"X86", [i16], 16,
+                               (sub GR16,  (sequence "R%uW", 16, 31))>;
+// GR32_NOREX2 - GR32 registers which do not require a REX2 prefix.
+def GR32_NOREX2 : RegisterClass<"X86", [i32], 32,
+                               (sub GR32,  (sequence "R%uD", 16, 31))>;
+// GR64_NOREX2 - GR64 registers which do not require a REX2 prefix.
+def GR64_NOREX2 : RegisterClass<"X86", [i64], 64,
+                               (sub GR64,  (sequence "R%u", 16, 31))>;
+}
 
 // GR32_NOSP - GR32 registers except ESP.
 def GR32_NOSP : RegisterClass<"X86", [i32], 32, (sub GR32, ESP)>;
@@ -506,6 +660,15 @@ def GR32_NOREX_NOSP : RegisterClass<"X86", [i32], 32,
 // GR64_NOREX_NOSP - GR64_NOREX registers except RSP.
 def GR64_NOREX_NOSP : RegisterClass<"X86", [i64], 64,
                                     (and GR64_NOREX, GR64_NOSP)>;
+let GeneratePressureSet = 0 in {
+// GR32_NOREX2_NOSP - GR32_NOREX2 registers except ESP.
+def GR32_NOREX2_NOSP : RegisterClass<"X86", [i32], 32,
+                                    (sub GR32_NOREX2, ESP)>;
+
+// GR64_NOREX2_NOSP - GR64_NOREX2 registers except RSP, RIP.
+def GR64_NOREX2_NOSP : RegisterClass<"X86", [i64], 64,
+                                    (sub GR64_NOREX2, RSP, RIP)>;
+}
 
 // Register classes used for ABIs that use 32-bit address accesses,
 // while using the whole x84_64 ISA.
diff --git a/llvm/test/CodeGen/MIR/X86/inline-asm-registers.mir b/llvm/test/CodeGen/MIR/X86/inline-asm-registers.mir
index f92d49cabdcdae3..2ac4d7cccac079b 100644
--- a/llvm/test/CodeGen/MIR/X86/inline-asm-registers.mir
+++ b/llvm/test/CodeGen/MIR/X86/inline-asm-registers.mir
@@ -28,8 +28,8 @@ body: |
     liveins: $rdi, $rsi
 
   ; CHECK-LABEL: name: test
-  ; CHECK: INLINEASM &foo, 0 /* attdialect */, 4521994 /* regdef:GR64 */, def $rsi, 4521994 /* regdef:GR64 */, def dead $rdi,
-    INLINEASM &foo, 0, 4521994, def $rsi, 4521994, def dead $rdi, 2147549193, killed $rdi, 2147483657, killed $rsi, 12, implicit-def dead early-clobber $eflags
+  ; CHECK: INLINEASM &foo, 0 /* attdialect */, 4784138 /* regdef:GR64 */, def $rsi, 4784138 /* regdef:GR64 */, def dead $rdi,
+    INLINEASM &foo, 0, 4784138, def $rsi, 4784138, def dead $rdi, 2147549193, killed $rdi, 2147483657, killed $rsi, 12, implicit-def dead early-clobber $eflags
     $rax = MOV64rr killed $rsi
     RET64 killed $rax
 ...
@@ -45,8 +45,8 @@ body: |
 
   ; Verify that the register ties are preserved.
   ; CHECK-LABEL: name: test2
-  ; CHECK: INLINEASM &foo, 0 /* attdialect */, 4521994 /* regdef:GR64 */, def $rsi, 4521994 /* regdef:GR64 */, def dead $rdi, 2147549193 /* reguse tiedto:$1 */, killed $rdi(tied-def 5), 2147483657 /* reguse tiedto:$0 */, killed $rsi(tied-def 3), 12 /* clobber */, implicit-def dead early-clobber $eflags
-    INLINEASM &foo, 0, 4521994, def $rsi, 4521994, def dead $rdi, 2147549193, killed $rdi(tied-def 5), 2147483657, killed $rsi(tied-def 3), 12, implic...
[truncated]

llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h Outdated Show resolved Hide resolved
llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h Outdated Show resolved Hide resolved
llvm/lib/Target/X86/X86InstrInfo.cpp Show resolved Hide resolved
llvm/lib/Target/X86/X86RegisterInfo.td Show resolved Hide resolved
llvm/lib/Target/X86/X86RegisterInfo.td Outdated Show resolved Hide resolved
llvm/lib/Target/X86/X86RegisterInfo.td Outdated Show resolved Hide resolved
llvm/lib/Target/X86/X86RegisterInfo.td Show resolved Hide resolved
llvm/test/CodeGen/X86/musttail-varargs.ll Show resolved Hide resolved
@nickdesaulniers
Copy link
Member

inline-asm-registers.mir, statepoint-invoke-ra-enter-at-end.mir:
The immediate (TargetInstrInfo.cpp:1612) used for the regdef/reguse is the encoding for the register
class in the enum generated by tablegen. This encoding will change
any time a new register class is added. Since the number is part
of the input, this means it can become stale.

yep, those changes LGTM!

@KanRobert
Copy link
Contributor Author

Ping?

llvm/lib/Target/X86/X86InstrInfo.cpp Show resolved Hide resolved
llvm/lib/Target/X86/X86InstrInfo.h Show resolved Hide resolved
Comment on lines +161 to +164
case X86::GR8_NOREX2RegClassID:
case X86::GR16_NOREX2RegClassID:
case X86::GR32_NOREX2RegClassID:
case X86::GR64_NOREX2RegClassID:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me when we need to distinguish X86::GR8_NOREX2RegClassID from X86::GR8RegClassID and when not. We have some other places, e.g., here that using X86::GRxxRegClassID, shouldn't need to update with X86::GRxx_NOREX2RegClassID
Besides, we also have some place to createVirtualRegister(&X86::GRxxRegClass), should they need to update too?

Copy link
Contributor Author

@KanRobert KanRobert Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have comments in X86BaseInfo.h about when we need to distinguish them. PREFETCH instructions are in map TB and they can use r16-r31, so X86::GRxxRegClassID does not need to be updated.

Yes, createVirtualRegister(&X86::GRxxRegClass) needs to be updated if the instruction can not encode r16-r31. But I haven't found such place so far.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you update in this patch or a follow up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look through one by one, but here seems have risk since it itrates different instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no createVirtualRegister(&X86::GRxxRegClass) for instructions that can not encode EGPR so far. If there is any in the future, we will directly use createVirtualRegister(&X86::GRxx_NOREX2RegClass) at those places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the risk?

    Register Reg = MRI->createVirtualRegister(
        TII->getRegClass(TII->get(DstOpcode), 0, MRI->getTargetRegisterInfo(),
                         *MBB->getParent()));
    MachineInstrBuilder Bld = BuildMI(*MBB, MI, DL, TII->get(DstOpcode), Reg);

The code calls getRegClass to get the register class and then build the machine instruction with the same opcode. It looks safe to me.

In X86, (If I remember correctly) pseudo instruction COPY is either a MOV or KMOV. Both of them can encode r16-31.

llvm/lib/Target/X86/X86RegisterInfo.td Show resolved Hide resolved
llvm/test/CodeGen/X86/apx/no-rex2-general.ll Show resolved Hide resolved
Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please wait one more day for reviewers.

Copy link
Contributor

@e-kud e-kud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ clang test.c -S -o - -m32
...
test.c:2:7: error: register %r27 is only available in 64-bit mode
    2 |   asm("addq %r27, %r29");
      |       ^

But there is no error that egpr is not enabled

Also I've tried to use m[no-]egpr. Is it intended to not support it yet?
UPD: the latter is clang's part, I guess it will be fixed in next patches.

@KanRobert
Copy link
Contributor Author

$ clang test.c -S -o - -m32
...
test.c:2:7: error: register %r27 is only available in 64-bit mode
    2 |   asm("addq %r27, %r29");
      |       ^

But there is no error that egpr is not enabled

Also I've tried to use m[no-]egpr. Is it intended to not support it yet? UPD: the latter is clang's part, I guess it will be fixed in next patches.

The assembler/disassembler and clang support will be supported in following patches. I try not to make a patch too big。

Copy link
Contributor

@e-kud e-kud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KanRobert KanRobert merged commit feea5db into llvm:main Oct 10, 2023
3 checks passed
nikic added a commit that referenced this pull request Oct 10, 2023
This reverts commit feea5db.

This causes significant compile-time regressions, even if EGPR is
not used.
@nikic
Copy link
Contributor

nikic commented Oct 10, 2023

I have reverted this change, because it causes large compile-time regressions: http://llvm-compile-time-tracker.com/compare.php?from=b5dffd4957dfb58c73e168a3d9b6967f03b23a6c&to=feea5db01360b477b8cf2df03abfa9fc986633d5&stat=instructions%3Au

Please make sure that EGPR enablement does not materially affect compile-time for people who do not enable the EGPR target feature (= nearly everyone).

@KanRobert
Copy link
Contributor Author

I have reverted this change, because it causes large compile-time regressions: http://llvm-compile-time-tracker.com/compare.php?from=b5dffd4957dfb58c73e168a3d9b6967f03b23a6c&to=feea5db01360b477b8cf2df03abfa9fc986633d5&stat=instructions%3Au

Please make sure that EGPR enablement does not materially affect compile-time for people who do not enable the EGPR target feature (= nearly everyone).

Thanks for the inform. Could I know how I can check if the updated patch may cause compile-time regression? @nikic

@KanRobert
Copy link
Contributor Author

KanRobert commented Oct 16, 2023

Hi @nikic, I reproduced the regression for sqlite3 and did some investigation. The 0.4% instruction count regression was introduced by definitions of new registers. As long as we introduce definitions in the td file,

def R16B : X86Reg<"r16b", 16>;

even if we do not add these registers to any register class, regression will occur. For example, when we collect the instruction counts by command

valgrind --tool=callgrind clang -DNDEBUG  -O3   -w -Werror=date-time -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DSQLITE_OMIT_LOAD_EXTENSION=1 -DSQLITE_THREADSA    FE=0 -I. -MD -MT MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/shell.c.o -MF MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/shell.c.o.d -o MultiSource/Applications/sqlite3/CMakeFiles/sqlite3.dir/shell.c.o -c /export/users/skan/test-suite/MultiSource/Applications/sqlite3/shell.c

the new added registers will introduce 0.6% regression in total. Here is the statistic:

Change instruction count regression
baseline 2,252,104,105 0%
+ R16B-R31B 2,256,497,475 0.17%
+ R16BH-R31BH 2,259,699,801 0.26%
+ R16WH-R31WH 2,261,163,600 0.4%
+ R16W-R31W 2,262,719,972 0.44%
+ R16D-R31D 2,264,204,477 0.53%
+ R16-R31 2,267,003,623 0.6%

The extra instructions mainly come from the iteration over these registers like

bool LiveVariables::runOnMachineFunction(MachineFunction &mf) {
...
  const unsigned NumRegs = TRI->getNumRegs();
...
  for (MachineBasicBlock *MBB : depth_first_ext(Entry, Visited)) {
    runOnBlock(MBB, NumRegs);
...
}

void LiveVariables::runOnBlock(MachineBasicBlock *MBB, const unsigned NumRegs) {
...
  // Loop over PhysRegDef / PhysRegUse, killing any registers that are
  // available at the end of the basic block.
  for (unsigned i = 0; i != NumRegs; ++i)
    if ((PhysRegDef[i] || PhysRegUse[i]) && !LiveOuts.count(i))
      HandlePhysRegDef(i, nullptr, Defs);

}

NumRegs equals 292 w/o this PR and equals 388 (292+16*6) w/ this PR. The code does some stuff on registers even if the target does not support them. I think it's a target-independent issue and not clear how to fix it.

Do you have any ideas or could we ignore this regression for the time being? cc @phoebewang @RKSimon @topperc

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 16, 2023

Do we have no choice but to iterate over the EGPRs even for cpus that don't support them? Why does TRI::getNumRegs() always include them?

What in particular about that HandlePhysRegDef loop is causing the perf regression? Surely at the moment we're just getting as far as the (PhysRegDef[i] || PhysRegUse[i]) check which is always false, is that part that expensive?

@KanRobert
Copy link
Contributor Author

From the table above, we know that there are 15M extra instructions. (PhysRegDef[i] || PhysRegUse[i]) here contributes about 2M. HandlePhysRegDef does not cause the regression. Here is the statistic:

Function name extra instructions
__memset_avx2_unaligned_erms 3M
LiveVariables::runOnBlock 2M
LiveRegUnits::removeRegsNotPreserved 2M
ReachingDefAnalysis::enterBasicBlock 2M
llvm::LiveVariables::HandleRegMask 1M
_int_malloc 1M
other 4M

All the regressions are about the iteration over EGPRs.

@KanRobert
Copy link
Contributor Author

Do we have no choice but to iterate over the EGPRs even for cpus that don't support them? Why does TRI::getNumRegs() always include them?

In current design, yes, we have to iterate over them.

/// Initialize MCRegisterInfo, called by TableGen
/// auto-generated routines. *DO NOT USE*.
void InitMCRegisterInfo(const MCRegisterDesc *D, unsigned NR, unsigned RA,
                        unsigned PC, const MCRegisterClass *C, unsigned NC,
                        const MCPhysReg (*RURoots)[2], unsigned NRU,
                        const int16_t *DL, const LaneBitmask *RUMS,
                        const char *Strings, const char *ClassStrings,
                        const uint16_t *SubIndices, unsigned NumIndices,
                        const SubRegCoveredBits *SubIdxRanges,
                        const uint16_t *RET) {
  Desc = D;
  NumRegs = NR;

The return value of TRI::getNumRegs() is a constant generated by tablgen, which is same for all cpus for one target.

@topperc
Copy link
Collaborator

topperc commented Oct 18, 2023

The return value of TRI::getNumRegs() is a constant generated by tablgen, which is same for all cpus for one target.

I assume getNumRegs is the total number of registers defined in the enum in X86GenRegisterInfo.inc and the new registers are not at the end of that enum?

@KanRobert
Copy link
Contributor Author

The return value of TRI::getNumRegs() is a constant generated by tablgen, which is same for all cpus for one target.

I assume getNumRegs is the total number of registers defined in the enum in X86GenRegisterInfo.inc and the new registers are not at the end of that enum?

Yes, you are right.

@KanRobert
Copy link
Contributor Author

I am going to reland this PR tomorrow if no objection. @nikic @RKSimon @topperc @phoebewang

@nikic
Copy link
Contributor

nikic commented Oct 19, 2023

I am going to reland this PR tomorrow if no objection.

I don't think this should be relanded without addressing the issue. Even if it is a pre-existing one, this patch makes it worse.

@jayfoad How far are we from removing LiveVariables completely? I saw #66784 waiting for review -- is that the last step needed to make TwoAddressInstruction work on LiveIntervals instead? Is any further work needed after that?

@jayfoad
Copy link
Contributor

jayfoad commented Oct 19, 2023

@jayfoad How far are we from removing LiveVariables completely?

Hard to say until we get there. The last time I tried enabling -early-live-intervals by default there were still a couple (but only a couple) of crashes/assertions caused by TwoAddressInstruction not updating LiveIntervals correctly.

After that, there is a huge amount of churn in the lit tests which makes it hard to spot potential problems. I pushed #67038 to mitigate that but reverted due to problems with LiveDebugVariables which I am still looking at.

After that, who knows? There could be more problems lurking.

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 19, 2023

I am going to reland this PR tomorrow if no objection. @nikic @RKSimon @topperc @phoebewang

@KanRobert We haven't really addressed the pref regression issues yet, just identified them - I'm sorry but it looks like there's some yak shaving to be done :(

KanRobert added a commit that referenced this pull request Nov 1, 2023
…0222)

* Introduce field `PositionOrder` for class `Register` and
`RegisterTuples`
* If register A's `PositionOrder` < register B's `PositionOrder`, then A
is placed before B in the enum in X86GenRegisterInfo.inc
* The new order of registers in the enum for X86 will be
      1. Registers before AVX512,
      2. AVX512 registers (X/YMM16-31, ZMM0-31, K registers)
      3. AMX registers (TMM)
      4.  APX registers (R16-R31)
* Add a new target hook `getNumSupportedRegs()` to return the number of
registers for the function (may overestimate).
* Replace `getNumRegs()` with `getNumSupportedRegs()` in LiveVariables
to eliminate iterations on unsupported registers

This patch can reduce 0.3% instruction count regression for sqlite3
during compile-stage (O3) by not iterating on APX registers
for #67702
KanRobert added a commit that referenced this pull request Nov 9, 2023
1. Map R16-R31 to DWARF registers 130-145.
2. Make R16-R31 caller-saved registers.
3. Make R16-31 allocatable only when feature EGPR is supported
4. Make R16-31 availabe for instructions in legacy maps 0/1 and EVEX
space, except XSAVE*/XRSTOR

RFC:

https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4

Explanations for some seemingly unrelated changes:

inline-asm-registers.mir, statepoint-invoke-ra-enter-at-end.mir:
The immediate (TargetInstrInfo.cpp:1612) used for the regdef/reguse is
the encoding for the register
  class in the enum generated by tablegen. This encoding will change
  any time a new register class is added. Since the number is part
  of the input, this means it can become stale.

seh-directive-errors.s:
   R16-R31 makes ".seh_pushreg 17" legal

musttail-varargs.ll:
It seems some LLVM passes use the number of registers rather the number
of allocatable registers as heuristic.

This PR is to reland #67702 after #70222 in order to reduce some
compile-time regression when EGPR is not used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants