AMDGPU/AsmParser: Add support for parsing symbol operands

[• tstellarAMD]

Summary

We can now reference symbols directly in operands, like this:
s_mov_b32 s0, global

[• tstellarAMD]

Add testcase.

[arsenm]

I thought symbols usually had some special prefix character to distinguish them

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

149 ↗

(On Diff #59772)

This looks redundant with the cast<>

1339 ↗

(On Diff #59772)

Typo difficlut

[• tstellarAMD]

Remove an extra call to Lex().

[• tstellarAMD]

Fix typo.

[• tstellarAMD]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

149 ↗

(On Diff #59787)

It's not redundant, because Expr could be any of the MCExpr sub-classes. For example in the added tests case, Expr is sometimes an MCBinaryExpr.

[vpykhtin]

LGTM.

[artem.tamazov]

Please address comment about simple tests and go ahead.

test/MC/AMDGPU/global-expr.s

7–8 ↗

(On Diff #59787)

I woudl be nice to add also tests for simple absolute expressions, like

.set foo 4
s_mov_b32 s0, foo+2 // should load 6 to s0

[• tstellarAMD]

In D21038#450845, @artem.tamazov wrote:

Please address comment about simple tests and go ahead.

There was an unrelated bug with these expression, so I've added the test with D21236, which also fixes the bug.

[artem.tamazov]

It seems that labels in branches were not anticipated in this patch.
The quickfix is under review, http://reviews.llvm.org/D22133.

Shall we try another approach for this feature; this one looks like a hack?..
Another problem (different story, but newertheless):

.set foo 40
s_branch foo

Branch offset will be something like (40-PC) but correct one should be +40, I guess. Shall we differentiate between labels and other kinds of symbols?

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

public:
   union {
     TokOp Tok;
     ImmOp Imm;
     RegOp Reg;
     const MCExpr *Expr;
   };
 
   bool isToken() const override {
-    return Kind == Token;
+    if (Kind == Token)
+      return true;
+
+    if (Kind != Expression || !Expr)
+      return false;
+
+    // When parsing operands, we can't always tell if something was meant to be
+    // a token, like 'gds', or an expression that references a global variable.
+    // In this case, we assume the string is an expression, and if we need to
+    // interpret is a token, then we treat the symbol name as the token.
+    return isa<MCSymbolRefExpr>(Expr);
   }
 
   bool isImm() const override {
     return Kind == Immediate;
   }
 
   bool isInlinableImm() const {
     if (!isImmTy(ImmTyNone)) {
bool isSCSrc32() const {
     return isInlinableImm() || isRegClass(AMDGPU::SReg_32RegClassID);
   }
 
   bool isSCSrc64() const {
     return isInlinableImm() || isRegClass(AMDGPU::SReg_64RegClassID);
   }
 
   bool isSSrc32() const {
-    return isImm() || isSCSrc32();
+    return isImm() || isSCSrc32() || isExpr();
   }
 
   bool isSSrc64() const {
     // TODO: Find out how SALU supports extension of 32-bit literals to 64 bits.
     // See isVSrc64().
     return isImm() || isSCSrc64();
   }
 
public:
   bool isSWaitCnt() const;
   bool isHwreg() const;
   bool isSendMsg() const;
   bool isMubufOffset() const;
   bool isSMRDOffset() const;
   bool isSMRDLiteralOffset() const;
   bool isDPPCtrl() const;
 
+  StringRef getExpressionAsToken() const {
+    assert(isExpr());
+    const MCSymbolRefExpr *S = cast<MCSymbolRefExpr>(Expr);
+    return S->getSymbol().getName();
+  }
+
+
   StringRef getToken() const {
+    assert(isToken());
+
+    if (Kind == Expression)
+      return getExpressionAsToken();
+
     return StringRef(Tok.Data, Tok.Length);
   }
 
   int64_t getImm() const {
     assert(isImm());
     return Imm.Val;
   }
 
public:
 
   void addRegOperands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::createReg(AMDGPU::getMCReg(getReg(), *Reg.STI)));
   }
 
   void addRegOrImmOperands(MCInst &Inst, unsigned N) const {
     if (isRegKind())
       addRegOperands(Inst, N);
+    else if (isExpr())
+      Inst.addOperand(MCOperand::createExpr(Expr));
     else
       addImmOperands(Inst, N);
   }
 
   void addRegOrImmWithInputModsOperands(MCInst &Inst, unsigned N) const {
     Modifiers Mods = getModifiers();
     Inst.addOperand(MCOperand::createImm(Mods.getModifiersOperand()));
     if (isRegKind()) {
if (ResTy == MatchOperand_Success || ResTy == MatchOperand_ParseFail ||
     return ResTy;
 
   ResTy = parseRegOrImm(Operands);
 
   if (ResTy == MatchOperand_Success)
     return ResTy;
 
   if (getLexer().getKind() == AsmToken::Identifier) {
+    // If this identifier is a symbol, we want to create an expression for it.
+    // It is a little difficult to distinguish between a symbol name, and
+    // an instruction flag like 'gds'.  In order to do this, we parse
+    // all tokens as expressions and then treate the symbol name as the token
+    // string when we want to interpret the operand as a token.
     const auto &Tok = Parser.getTok();
+    SMLoc S = Tok.getLoc();
+    const MCExpr *Expr = nullptr;
+    if (!Parser.parseExpression(Expr)) {
+      Operands.push_back(AMDGPUOperand::CreateExpr(Expr, S));
+      return MatchOperand_Success;
+    }
+
     Operands.push_back(AMDGPUOperand::CreateToken(Tok.getString(), Tok.getLoc()));
     Parser.Lex();
     return MatchOperand_Success;
   }
   return MatchOperand_NoMatch;
 }
 
 StringRef AMDGPUAsmParser::parseMnemonicSuffix(StringRef Name) {
unsigned AMDGPUAsmParser::validateTargetOperandClass(MCParsedAsmOperand &Op,
   case MCK_gds:
     return Operand.isGDS() ? Match_Success : Match_InvalidOperand;
   case MCK_glc:
     return Operand.isGLC() ? Match_Success : Match_InvalidOperand;
   case MCK_idxen:
     return Operand.isIdxen() ? Match_Success : Match_InvalidOperand;
   case MCK_offen:
     return Operand.isOffen() ? Match_Success : Match_InvalidOperand;
+  case MCK_SSrc32:
+    // When operands have expression values, they will return true for isToken,
+    // because it is not possible to distinguish between a token and an
+    // expression at parse time. MatchInstructionImpl() will always try to
+    // match an operand as a token, when isToken returns true, and when the
+    // name of the expression is not a valid token, the match will fail,
+    // so we need to handle it here.
+    return Operand.isSSrc32() ? Match_Success : Match_InvalidOperand;
   default: return Match_InvalidOperand;
   }
 }

llvm/trunk/test/MC/AMDGPU/expressions.s

+// RUN: llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s | FileCheck %s --check-prefix=VI
+
+
+.globl global
+.globl gds
+
+// Parse a global expression
+s_mov_b32 s0, global
+// VI: s_mov_b32 s0, global ; encoding: [0xff,0x00,0x80,0xbe,A,A,A,A]
+// VI-NEXT: ;   fixup A - offset: 4, value: global, kind: FK_PCRel_4
+
+// Use a token with the same name as a global
+ds_gws_init v2 gds
+// VI: ds_gws_init v2 gds ; encoding: [0x00,0x00,0x33,0xd8,0x02,0x00,0x00,0x00]
+
+// Use a global with the same name as a token
+s_mov_b32 s0, gds
+// VI: s_mov_b32 s0, gds ; encoding: [0xff,0x00,0x80,0xbe,A,A,A,A]
+// VI-NEXT: ;   fixup A - offset: 4, value: gds, kind: FK_PCRel_4
+
+// Use a binary expression
+s_mov_b32 s0, gds+4
+// VI: s_mov_b32 s0, gds+4 ; encoding: [0xff,0x00,0x80,0xbe,A,A,A,A]
+// VI-NEXT: ;   fixup A - offset: 4, value: gds+4, kind: FK_PCRel_4
+
+// Consecutive instructions with no blank line in between to make sure we
+// don't call Lex() too many times.
+s_add_u32 s0, s0, global+4
+s_addc_u32 s1, s1, 0
+// VI: s_add_u32 s0, s0, global+4
+// VI: s_addc_u32 s1, s1, 0