[AMDGPU][MC] Refactored exp tgt handling

[dp]

Summary

Summary of changes:

• Separated tgt encoding from parsing;
• Separated tgt decoding from printing;
• Improved errors handling;
• Disabled leading zeroes in index. The following code is no longer accepted: exp pos00 v3, v2, v1, v0

[foad]

Separated tgt encoding from parsing;
Separated tgt decoding from printing;

Is there a reason for this? Do you want to use the encoding/decoding logic from somewhere else?

[dp]

In D95216#2515318, @foad wrote:

Separated tgt encoding from parsing;
Separated tgt decoding from printing;

Is there a reason for this? Do you want to use the encoding/decoding logic from somewhere else?

No, this is an attempt to make code more manageable. I believe that encoding/decoding logic should be in one place rather than split between parser and instruction printer. I think that most encoding/decoding logic have already been moved to AMDGPUBaseInfo.

[MaskRay]

In your commit the message has just Reviewers:. The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless. Many people agree that both Reviewed by: & Differential Revision: should be present.

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/.git/hooks/pre-push to prevent accidental Summary:, Reviewers:, Subscribers: and Tags:.

[dp]

In D95216#2523007, @MaskRay wrote:

In your commit the message has just Reviewers:.

"git log" seems to display the list of reviewers correctly:

Reviewers: arsenm, rampitec, foad

When I open this commit on github, the reviewer list seems to be present as well:
https://github.com/llvm/llvm-project/commit/745064e36b8751548061cc72039dc039c566d175

What am I missing?

[MaskRay]

In D95216#2523107, @dp wrote:

In D95216#2523007, @MaskRay wrote:

In your commit the message has just Reviewers:.

"git log" seems to display the list of reviewers correctly:

Reviewers: arsenm, rampitec, foad

When I open this commit on github, the reviewer list seems to be present as well:
https://github.com/llvm/llvm-project/commit/745064e36b8751548061cc72039dc039c566d175

What am I missing?

arc diff created message does not have Reviewers:. arc amend can drop the tag as well.

[dp]

Thanks. I did not know that use of these tags have obsoleted.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

private:
 
   bool parseHwregBody(OperandInfoTy &HwReg,
                       OperandInfoTy &Offset,
                       OperandInfoTy &Width);
   bool validateHwreg(const OperandInfoTy &HwReg,
                      const OperandInfoTy &Offset,
                      const OperandInfoTy &Width);
 
-  OperandMatchResultTy parseExpTgtImpl(StringRef Str, uint8_t &Val);
   SMLoc getFlatOffsetLoc(const OperandVector &Operands) const;
   SMLoc getSMEMOffsetLoc(const OperandVector &Operands) const;
 
   SMLoc getOperandLoc(std::function<bool(const AMDGPUOperand&)> Test,
                       const OperandVector &Operands) const;
   SMLoc getImmLoc(AMDGPUOperand::ImmTy Type, const OperandVector &Operands) const;
   SMLoc getRegLoc(unsigned Reg, const OperandVector &Operands) const;
   SMLoc getLitLoc(const OperandVector &Operands) const;
Operands.push_back(AMDGPUOperand::CreateImm(this, AttrChan, SChan,
                                               AMDGPUOperand::ImmTyAttrChan));
   return MatchOperand_Success;
 }
 
 //===----------------------------------------------------------------------===//
 // exp
 //===----------------------------------------------------------------------===//
 
-OperandMatchResultTy AMDGPUAsmParser::parseExpTgtImpl(StringRef Str,
-                                                      uint8_t &Val) {
-  if (Str == "null") {
-    Val = Exp::ET_NULL;
-    return MatchOperand_Success;
-  }
-
-  if (Str.startswith("mrt")) {
-    Str = Str.drop_front(3);
-    if (Str == "z") { // == mrtz
-      Val = Exp::ET_MRTZ;
-      return MatchOperand_Success;
-    }
-
-    if (Str.getAsInteger(10, Val))
-      return MatchOperand_ParseFail;
-
-    if (Val > Exp::ET_MRT7)
-      return MatchOperand_ParseFail;
-
-    return MatchOperand_Success;
-  }
-
-  if (Str.startswith("pos")) {
-    Str = Str.drop_front(3);
-    if (Str.getAsInteger(10, Val))
-      return MatchOperand_ParseFail;
-
-    if (Val > (isGFX10Plus() ? 4 : 3))
-      return MatchOperand_ParseFail;
-
-    Val += Exp::ET_POS0;
-    return MatchOperand_Success;
-  }
-
-  if (isGFX10Plus() && Str == "prim") {
-    Val = Exp::ET_PRIM;
-    return MatchOperand_Success;
-  }
-
-  if (Str.startswith("param")) {
-    Str = Str.drop_front(5);
-    if (Str.getAsInteger(10, Val))
-      return MatchOperand_ParseFail;
-
-    if (Val >= 32)
-      return MatchOperand_ParseFail;
-
-    Val += Exp::ET_PARAM0;
-    return MatchOperand_Success;
-  }
-
-  return MatchOperand_ParseFail;
-}
-
 OperandMatchResultTy AMDGPUAsmParser::parseExpTgt(OperandVector &Operands) {
+  using namespace llvm::AMDGPU::Exp;
+
   StringRef Str;
   SMLoc S = getLoc();
 
   if (!parseId(Str))
     return MatchOperand_NoMatch;
 
-  uint8_t Val;
-  auto Res = parseExpTgtImpl(Str, Val);
-  if (Res != MatchOperand_Success) {
-    Error(S, "invalid exp target");
-    return Res;
+  unsigned Id = getTgtId(Str);
+  if (Id == ET_INVALID || !isSupportedTgtId(Id, getSTI())) {
+    Error(S, (Id == ET_INVALID) ?
+                "invalid exp target" :
+                "exp target is not supported on this GPU");
+    return MatchOperand_ParseFail;
   }
 
-  Operands.push_back(AMDGPUOperand::CreateImm(this, Val, S,
+  Operands.push_back(AMDGPUOperand::CreateImm(this, Id, S,
                                               AMDGPUOperand::ImmTyExpTgt));
   return MatchOperand_Success;
 }
 
 //===----------------------------------------------------------------------===//
 // parser helpers
 //===----------------------------------------------------------------------===//
 

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

void AMDGPUInstPrinter::printExpSrc3(const MCInst *MI, unsigned OpNo,
                                      const MCSubtargetInfo &STI,
                                      raw_ostream &O) {
   printExpSrcN(MI, OpNo, STI, O, 3);
 }
 
 void AMDGPUInstPrinter::printExpTgt(const MCInst *MI, unsigned OpNo,
                                     const MCSubtargetInfo &STI,
                                     raw_ostream &O) {
+  using namespace llvm::AMDGPU::Exp;
+
   // This is really a 6 bit field.
-  uint32_t Tgt = MI->getOperand(OpNo).getImm() & ((1 << 6) - 1);
+  uint32_t Id = MI->getOperand(OpNo).getImm() & ((1 << 6) - 1);
 
-  if (Tgt <= Exp::ET_MRT7)
-    O << " mrt" << Tgt - Exp::ET_MRT0;
-  else if (Tgt == Exp::ET_MRTZ)
-    O << " mrtz";
-  else if (Tgt == Exp::ET_NULL)
-    O << " null";
-  else if (Tgt >= Exp::ET_POS0 &&
-           Tgt <= uint32_t(isGFX10Plus(STI) ? Exp::ET_POS4 : Exp::ET_POS3))
-    O << " pos" << Tgt - Exp::ET_POS0;
-  else if (isGFX10Plus(STI) && Tgt == Exp::ET_PRIM)
-    O << " prim";
-  else if (Tgt >= Exp::ET_PARAM0 && Tgt <= Exp::ET_PARAM31)
-    O << " param" << Tgt - Exp::ET_PARAM0;
-  else {
-    // Reserved values 10, 11
-    O << " invalid_target_" << Tgt;
+  int Index;
+  StringRef TgtName;
+  if (getTgtName(Id, TgtName, Index) && isSupportedTgtId(Id, STI)) {
+    O << ' ' << TgtName;
+    if (Index >= 0)
+      O << Index;
+  } else {
+    O << " invalid_target_" << Id;
   }
 }
 
 static bool allOpsDefaultValue(const int* Ops, int NumOps, int Mod,
                                bool IsPacked, bool HasDstSel) {
   int DefaultValue = IsPacked && (Mod == SISrcMods::OP_SEL_1);
 
   for (int I = 0; I < NumOps; ++I) {

llvm/lib/Target/AMDGPU/SIDefines.h

enum Target {
   ET_NULL = 9,
   ET_POS0 = 12,
   ET_POS3 = 15,
   ET_POS4 = 16,          // GFX10+
   ET_POS_LAST = ET_POS4, // Highest pos used on any subtarget
   ET_PRIM = 20,          // GFX10+
   ET_PARAM0 = 32,
   ET_PARAM31 = 63,
+  ET_INVALID = 255,
 };
 
 } // namespace Exp
 } // namespace AMDGPU
 
 #define R_00B028_SPI_SHADER_PGM_RSRC1_PS                                0x00B028
 #define   S_00B028_VGPRS(x)                                           (((x) & 0x3F) << 0)
 #define   S_00B028_SGPRS(x)                                           (((x) & 0x0F) << 6)

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 
 LLVM_READNONE
 StringRef getHwreg(unsigned Id, const MCSubtargetInfo &STI);
 
 void decodeHwreg(unsigned Val, unsigned &Id, unsigned &Offset, unsigned &Width);
 
 } // namespace Hwreg
 
+namespace Exp {
+
+bool getTgtName(unsigned Id, StringRef &Name, int &Index);
+
+LLVM_READONLY
+unsigned getTgtId(const StringRef Name);
+
+LLVM_READNONE
+bool isSupportedTgtId(unsigned Id, const MCSubtargetInfo &STI);
+
+} // namespace Exp
+
 namespace MTBUFFormat {
 
 LLVM_READNONE
 int64_t encodeDfmtNfmt(unsigned Dfmt, unsigned Nfmt);
 
 void decodeDfmtNfmt(unsigned Format, unsigned &Dfmt, unsigned &Nfmt);
 
 int64_t getDfmt(const StringRef Name);

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

void decodeHwreg(unsigned Val, unsigned &Id, unsigned &Offset, unsigned &Width) {
   Id = (Val & ID_MASK_) >> ID_SHIFT_;
   Offset = (Val & OFFSET_MASK_) >> OFFSET_SHIFT_;
   Width = ((Val & WIDTH_M1_MASK_) >> WIDTH_M1_SHIFT_) + 1;
 }
 
 } // namespace Hwreg
 
 //===----------------------------------------------------------------------===//
+// exp tgt
+//===----------------------------------------------------------------------===//
+
+namespace Exp {
+
+struct ExpTgt {
+  StringLiteral Name;
+  unsigned Tgt;
+  unsigned MaxIndex;
+};
+
+static constexpr ExpTgt ExpTgtInfo[] = {
+  {{"null"},  ET_NULL,    0},
+  {{"mrtz"},  ET_MRTZ,    0},
+  {{"prim"},  ET_PRIM,    0},
+  {{"mrt"},   ET_MRT0,    7},
+  {{"pos"},   ET_POS0,    7},
+  {{"param"}, ET_PARAM0, 31},
+};
+
+bool getTgtName(unsigned Id, StringRef &Name, int &Index) {
+  for (const ExpTgt &Val : ExpTgtInfo) {
+    if (Val.Tgt <= Id && Id <= Val.Tgt + Val.MaxIndex) {
+      Index = (Val.MaxIndex == 0) ? -1 : (Id - Val.Tgt);
+      Name = Val.Name;
+      return true;
+    }
+  }
+  return false;
+}
+
+unsigned getTgtId(const StringRef Name) {
+
+  for (const ExpTgt &Val : ExpTgtInfo) {
+    if (Val.MaxIndex == 0 && Name == Val.Name)
+      return Val.Tgt;
+
+    if (Val.MaxIndex > 0 && Name.startswith(Val.Name)) {
+      StringRef Suffix = Name.drop_front(Val.Name.size());
+
+      unsigned Id;
+      if (Suffix.getAsInteger(10, Id) || Id > Val.MaxIndex)
+        return ET_INVALID;
+
+      // Disable leading zeroes
+      if (Suffix.size() > 1 && Suffix[0] == '0')
+        return ET_INVALID;
+
+      return Val.Tgt + Id;
+    }
+  }
+  return ET_INVALID;
+}
+
+bool isSupportedTgtId(unsigned Id, const MCSubtargetInfo &STI) {
+  return (Id != ET_POS4 && Id != ET_PRIM) || isGFX10Plus(STI);
+}
+
+} // namespace Exp
+
+//===----------------------------------------------------------------------===//
 // MTBUF Format
 //===----------------------------------------------------------------------===//
 
 namespace MTBUFFormat {
 
 int64_t getDfmt(const StringRef Name) {
   for (int Id = DFMT_MIN; Id <= DFMT_MAX; ++Id) {
     if (Name == DfmtSymbolic[Id])

llvm/test/MC/AMDGPU/exp-err.s

 // RUN: not llvm-mc -arch=amdgcn %s 2>&1 | FileCheck -check-prefix=GCN --implicit-check-not=error: %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=tonga %s 2>&1 | FileCheck -check-prefix=GCN --implicit-check-not=error: %s
 
 exp mrt8 v3, v2, v1, v0
 // GCN: :5: error: invalid exp target
 
 exp pos4 v3, v2, v1, v0
-// GCN: :5: error: invalid exp target
+// GCN: :5: error: exp target is not supported on this GPU
 
 exp param32 v3, v2, v1, v0
 // GCN: :5: error: invalid exp target
 
 exp invalid_target_10 v3, v2, v1, v0
 // GCN: :5: error: invalid exp target
 
 exp invalid_target_10 v3, v2, v1, v0 done

llvm/test/MC/AMDGPU/exp-gfx10.s

 // RUN: not llvm-mc -arch=amdgcn -mcpu=verde %s 2>&1 | FileCheck -check-prefix=SIVI --implicit-check-not=error: %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=tonga %s 2>&1 | FileCheck -check-prefix=SIVI --implicit-check-not=error: %s
 // RUN: llvm-mc -arch=amdgcn -mcpu=gfx1010 -show-encoding %s | FileCheck -check-prefix=GFX10 %s
 
 exp prim v1, off, off, off
-// SIVI: :5: error: invalid exp target
+// SIVI: :5: error: exp target is not supported on this GPU
 // GFX10: exp prim v1, off, off, off ; encoding: [0x41,0x01,0x00,0xf8,0x01,0x00,0x00,0x00]
 
 exp prim v2, v3, off, off
-// SIVI: :5: error: invalid exp target
+// SIVI: :5: error: exp target is not supported on this GPU
 // GFX10: exp prim v2, v3, off, off ; encoding: [0x43,0x01,0x00,0xf8,0x02,0x03,0x00,0x00]
 
 exp pos4 v4, v3, v2, v1
-// SIVI: error: invalid exp target
+// SIVI: error: exp target is not supported on this GPU
 // GFX10: exp pos4 v4, v3, v2, v1 ; encoding: [0x0f,0x01,0x00,0xf8,0x04,0x03,0x02,0x01]

llvm/test/MC/AMDGPU/gfx10_err_pos.s

 //==============================================================================
 // invalid exp target
 
 exp invalid_target_10 v3, v2, v1, v0
 // CHECK: error: invalid exp target
 // CHECK-NEXT:{{^}}exp invalid_target_10 v3, v2, v1, v0
 // CHECK-NEXT:{{^}}    ^
 
+exp pos00 v3, v2, v1, v0
+// CHECK: error: invalid exp target
+// CHECK-NEXT:{{^}}exp pos00 v3, v2, v1, v0
+// CHECK-NEXT:{{^}}    ^
+
 //==============================================================================
 // invalid immediate: only 16-bit values are legal
 
 s_setreg_b32  0x1f803, s2
 // CHECK: error: invalid immediate: only 16-bit values are legal
 // CHECK-NEXT:{{^}}s_setreg_b32  0x1f803, s2
 // CHECK-NEXT:{{^}}              ^
 

llvm/test/MC/AMDGPU/gfx9_err_pos.s

 // duplicate VGPR index mode
 
 s_set_gpr_idx_on s0, gpr_idx(SRC0,DST,SRC1,DST)
 // CHECK: error: duplicate VGPR index mode
 // CHECK-NEXT:{{^}}s_set_gpr_idx_on s0, gpr_idx(SRC0,DST,SRC1,DST)
 // CHECK-NEXT:{{^}}                                           ^
 
 //==============================================================================
+// exp target is not supported on this GPU
+
+exp pos4 v4, v3, v2, v1
+// CHECK: error: exp target is not supported on this GPU
+// CHECK-NEXT:{{^}}exp pos4 v4, v3, v2, v1
+// CHECK-NEXT:{{^}}    ^
+
+//==============================================================================
 // expected a 12-bit unsigned offset
 
 flat_load_dword v1, v[3:4] offset:-1
 // CHECK: error: expected a 12-bit unsigned offset
 // CHECK-NEXT:{{^}}flat_load_dword v1, v[3:4] offset:-1
 // CHECK-NEXT:{{^}}                           ^
 
 flat_load_dword v1, v[3:4] offset:4096