[X86][AsmParser] re-introduce 'offset' operator

[coby]

Summary

Amend MS offset operator implementation, to more closely fit with its MS counterpart:

1. InlineAsm: evaluate non-local source entities to their (address) location
2. Provide a mean with which one may acquire the address of an assembly label via MS syntax, rather than yielding a memory reference (i.e. "offset asm_label" and "$asm_label" should be synonymous
3. address PR32530

Presented diffs are relative to D37412

InlineAsm tests: D37466

[coby]

Adding @mcrosier , as he originally introduced the feature

[coby]

ping

[rnk]

test/MC/X86/pr32530.s

6

Can you add some more tests? I think this combines with everything: scale, index, base, segment, and integer displacement.

[coby]

sure, will update shortly

[coby]

Minimal source changes - introduced 'new' enum values definitions as adapted on dependent change (D37412)
Added few more assembly level testing, as requested by @rnk
Minimized 'offset_operator.ll' test-case, and auto-updated it checks

[RKSimon]

@coby What is happening with this patch?

[coby]

In D37461#913703, @RKSimon wrote:

@coby What is happening with this patch?

currently suspended due to a more dire matters.
nonetheless - as far as functionality is to be considered, and unless any of the listed reviewers will object, this one require no further development.

[avt77]

@coby, is it possible to commit this patch? I'd like to fix https://bugs.llvm.org/show_bug.cgi?id=35115 based on this source.

[thakis]

Thanks for working on this! I'd be happy to see this fixed. It also likely helps with PR36676.

I patched it in to play with it (it still applies with just two hunks not applying via patch, but manually applying them seems easy.) I got an assert on this code (which is from Windows.h):

$ cat repro.ii
void Int64ShrlMod32(unsigned __int64 Value) {
  __asm {
    mov edx, dword ptr [Value+4]
  }
}
$ bin/clang-7.0 -cc1 -triple i386-pc-windows-msvc19.11.0 -S -disable-free -main-file-name moved.i -mrelocation-model static -mthread-model posix -relaxed-aliasing -fmath-errno -masm-verbose -mconstructor-aliases -target-cpu pentium4 -mllvm -x86-asm-syntax=intel -D_MT -flto-visibility-public-std --dependent-lib=libcmt --dependent-lib=oldnames -fms-volatile -fdiagnostics-format msvc -dwarf-column-info -debugger-tuning=gdb -target-linker-version 305 -momit-leaf-frame-pointer -ffunction-sections -coverage-notes-file /Users/thakis/src/llvm-build-goma/moved.gcno -resource-dir /Users/thakis/src/llvm-build-goma/lib/clang/7.0.0 -Os -Wno-msvc-not-found -WCL4 -fdebug-compilation-dir /Users/thakis/src/llvm-build-goma -ferror-limit 19 -fmessage-length 254 -fno-use-cxa-atexit -fms-extensions -fms-compatibility -fms-compatibility-version=19.11 -fdelayed-template-parsing -fobjc-runtime=gcc -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp -o moved.asm -x cpp-output repro.ii
Assertion failed: (AR.IntelExp.isValid() && "cannot write invalid intel expression"), function parseMSInlineAsm, file /Users/thakis/src/llvm-rw/lib/MC/MCParser/AsmParser.cpp, line 5709.
Stack dump:
0.	Program arguments: /Users/thakis/src/llvm-build-goma/bin/clang-7.0 -cc1 -triple i386-pc-windows-msvc19.11.0 -S -disable-free -main-file-name moved.i -mrelocation-model static -mthread-model posix -relaxed-aliasing -fmath-errno -masm-verbose -mconstructor-aliases -target-cpu pentium4 -mllvm -x86-asm-syntax=intel -D_MT -flto-visibility-public-std --dependent-lib=libcmt --dependent-lib=oldnames -fms-volatile -fdiagnostics-format msvc -dwarf-column-info -debugger-tuning=gdb -target-linker-version 305 -momit-leaf-frame-pointer -ffunction-sections -coverage-notes-file /Users/thakis/src/llvm-build-goma/moved.gcno -resource-dir /Users/thakis/src/llvm-build-goma/lib/clang/7.0.0 -Os -Wno-msvc-not-found -WCL4 -fdebug-compilation-dir /Users/thakis/src/llvm-build-goma -ferror-limit 19 -fmessage-length 254 -fno-use-cxa-atexit -fms-extensions -fms-compatibility -fms-compatibility-version=19.11 -fdelayed-template-parsing -fobjc-runtime=gcc -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp -o moved.asm -x cpp-output repro.ii 
1.	repro.ii:5:1: current parser token '}'
2.	repro.ii:1:45: parsing function body 'Int64ShrlMod32'
3.	repro.ii:1:45: in compound statement ('{}')
0  clang-7.0                0x000000010d82ebc8 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40
1  clang-7.0                0x000000010d82ddc5 llvm::sys::RunSignalHandlers() + 85
2  clang-7.0                0x000000010d82f1d2 SignalHandler(int) + 258
3  libsystem_platform.dylib 0x00007fffc1fd2b3a _sigtramp + 26
4  libsystem_platform.dylib 0x00007fff532a277c _sigtramp + 2435644508
5  libsystem_c.dylib        0x00007fffc1e57420 abort + 129
6  libsystem_c.dylib        0x00007fffc1e1e893 basename_r + 0
7  clang-7.0                0x000000010d5320cd (anonymous namespace)::AsmParser::parseMSInlineAsm(void*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, unsigned int&, unsigned int&, llvm::SmallVectorImpl<std::__1::pair<void*, bool> >&, llvm::SmallVectorImpl<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >&, llvm::SmallVectorImpl<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >&, llvm::MCInstrInfo const*, llvm::MCInstPrinter const*, llvm::MCAsmParserSemaCallback&) + 7981
8  clang-7.0                0x000000010e936e52 clang::Parser::ParseMicrosoftAsmStatement(clang::SourceLocation) + 5746
9  clang-7.0                0x000000010e92bf66 clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::AllowedConstructsKind, clang::SourceLocation*, clang::Parser::ParsedAttributesWithRange&) + 1238
10 clang-7.0                0x000000010e92b938 clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::AllowedConstructsKind, clang::SourceLocation*) + 264
11 clang-7.0                0x000000010e933739 clang::Parser::ParseCompoundStatementBody(bool) + 1497

[RKSimon]

@coby Are you still looking as this? If not (@coby hasn't been active on phab since 2017) @thakis would you be in a position to commandeer this and complete it?

[thakis]

In D37461#1150631, @RKSimon wrote:

@coby Are you still looking as this? If not (@coby hasn't been active on phab since 2017) @thakis would you be in a position to commandeer this and complete it?

I can try, but I don't know this code well. I've uploaded a rebased but otherwise unmodified version at https://reviews.llvm.org/D49648

The crash I posted above is because IntelExpr() used to map a Scale of 0 to 1. If I add that back in, my crash goes away.

However, offset is also supposed to work with quoted symbols. If I allow that like so:

+  if (!isParsingInlineAsm()) {
+    if ((getTok().isNot(AsmToken::Identifier) &&
+         getTok().isNot(AsmToken::String)) ||
+        getParser().parsePrimaryExpr(Val, End))
+      return Error(Start, "unexpected token!");

then one of the two examples in PR36676 starts working, but the other asserts due to an unknown relocation. The problem is that offset is modeled as a X86Operand::CreateImm() with a fixup, but since intel asm instrs don't have sizes the logic sees that the imm is 0 and hence creates a 16-bit imm and we then get a 2 byte reloc, which aren't implemented. This is wrong anyhow since push offset foo should create a pointer size relocation. X86AsmParser::ParseIntelOperand() could not create an imm but a memory ref if SM.isOffsetOperator() and that makes things go, but then the push is encoded with a PUSH32rmm instead of a PUSH32i8, which is a byte larger and different from what clang-cl writes if you don't pass /FA, so I'd like to fix that too. I haven't yet figured out how :-)

[craig.topper]

Ping @thakis do you have any updates on this?

[craig.topper]

Is this enough to keep using CreateImm and fix the push issue

@@ -3163,7 +3202,7 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
       // If it's not a constant fall through and let remainder take care of it.
       const auto *CE = dyn_cast<MCConstantExpr>(X86Op->getImm());
       unsigned Size = getPointerWidth();
-      if (CE &&
+      if (!CE ||
           (isIntN(Size, CE->getValue()) || isUIntN(Size, CE->getValue()))) {
         SmallString<16> Tmp;
         Tmp += Base;

This extends the hack we have to use pointer sized push to also do it when the immediate can't be resolved to a constant.

[craig.topper]

@thakis, ping

[thakis]

(I rebased this again and uploaded it with the assert fix at D49648. A few other tests fail in check-clang with it, and apparently clang-cl /FA -m32 no longer writes asm with offset, so some of my old tests no longer work. Will look more over the weekend.)

[thakis]

There are at least 3 things missing to get check-clang to work:

1. clang currently supports offset localvar, which cl.exe doesn't support. This is currently rewritten to the stack address; after this patch it instead becomes offset localvar but localvar isn't an existing symbol of course. So for locals, this would still have to do the rewrite.

2. For offset Foo::bar, clang used to rewrite that to a local symbol, not it just prints offset Foo::bar – I think we probably need to put the mangled name there instead.

3. For __asm mov [eax], offset var we get ambiguous operand size for instruction 'mov' now; that needs fixing.

I don't know if I'll have time soon to look into these.

[epastor]

In D37461#1383242, @thakis wrote:

There are at least 3 things missing to get check-clang to work:

1. clang currently supports offset localvar, which cl.exe doesn't support. This is currently rewritten to the stack address; after this patch it instead becomes offset localvar but localvar isn't an existing symbol of course. So for locals, this would still have to do the rewrite.

2. For offset Foo::bar, clang used to rewrite that to a local symbol, not it just prints offset Foo::bar – I think we probably need to put the mangled name there instead.

3. For __asm mov [eax], offset var we get ambiguous operand size for instruction 'mov' now; that needs fixing.

I've got a draft that fixes (1) and (2), handling both as operands. It avoids having to special-case local symbols too deeply, handling everything as an operand - local symbols have their location on the stack passed in through a register (and so can't be included in compound expressions), global symbols come in as an immediate (and so can be included in compound expressions).

(3) is interesting. Looking at https://godbolt.org/z/QQ9ASx, currently clang rewrites offset x to an explicitly pointer-sized register, with the choice of register hard-wired to RBX/EBX. Now that I'm handling local symbol offset as an operand, we get operand size ambiguity... which seems appropriate to me. Would it be reasonable to consider this the new correct behavior, or do we need to preserve it as is?

[thakis]

This landed in https://reviews.llvm.org/D71436

include/llvm/MC/MCParser/MCTargetAsmParser.h

 
 // Represnt the various parts which makes up an intel expression,
 // used for emitting compound intel expressions
 struct IntelExpr {
   bool NeedBracs;
   int64_t Imm;
   StringRef BaseReg;
   StringRef IndexReg;
+  StringRef OffsetName;
   unsigned Scale;
 
-  IntelExpr(bool needBracs = false) : NeedBracs(needBracs), Imm(0),
-    BaseReg(StringRef()), IndexReg(StringRef()),
-    Scale(1) {}
-  // Compund immediate expression
-  IntelExpr(int64_t imm, bool needBracs) : IntelExpr(needBracs) {
-    Imm = imm;
-  }
-  // [Reg + ImmediateExpression]
-  // We don't bother to emit an immediate expression evaluated to zero
-  IntelExpr(StringRef reg, int64_t imm = 0, unsigned scale = 0,
-    bool needBracs = true) :
-    IntelExpr(imm, needBracs) {
-    IndexReg = reg;
-    if (scale)
-      Scale = scale;
-  }
-  // [BaseReg + IndexReg * ScaleExpression + ImmediateExpression]
-  IntelExpr(StringRef baseReg, StringRef indexReg, unsigned scale = 0,
-    int64_t imm = 0, bool needBracs = true) :
-    IntelExpr(indexReg, imm, scale, needBracs) {
-    BaseReg = baseReg;
-  }
+  IntelExpr() : NeedBracs(false), Imm(0), BaseReg(StringRef()),
+                IndexReg(StringRef()), OffsetName(StringRef()), Scale(1) {}
+  // [BaseReg + IndexReg * ScaleExpression + OFFSET name + ImmediateExpression]
+  IntelExpr(StringRef baseReg, StringRef indexReg, unsigned scale,
+            StringRef offsetName, int64_t imm, bool needBracs) :
+    NeedBracs(needBracs), Imm(imm), BaseReg(baseReg), IndexReg(indexReg),
+    OffsetName(offsetName), Scale(scale) {}
   bool hasBaseReg() const {
     return BaseReg.size();
   }
   bool hasIndexReg() const {
     return IndexReg.size();
   }
   bool hasRegs() const {
     return hasBaseReg() || hasIndexReg();
   }
+  bool hasOffset() const {
+    return OffsetName.size();
+  }
+  // Normally we won't emit immediates unconditionally,
+  // unless we've got no other components
+  bool emitImm() const {
+    return !(hasRegs() || hasOffset());
+  }
   bool isValid() const {
     return (Scale == 1) ||
            (hasIndexReg() && (Scale == 2 || Scale == 4 || Scale == 8));
   }
 };
 
 struct AsmRewrite {
   AsmRewriteKind Kind;

lib/MC/MCParser/AsmParser.cpp

case AOK_IntelExpr:
       if (AR.IntelExp.NeedBracs)
         OS << "[";
       if (AR.IntelExp.hasBaseReg())
         OS << AR.IntelExp.BaseReg;
       if (AR.IntelExp.hasIndexReg())
         OS << (AR.IntelExp.hasBaseReg() ? " + " : "")
            << AR.IntelExp.IndexReg;
       if (AR.IntelExp.Scale > 1)
-          OS << " * $$" << AR.IntelExp.Scale;
-      if (AR.IntelExp.Imm || !AR.IntelExp.hasRegs())
-        OS << (AR.IntelExp.hasRegs() ? " + $$" : "$$") << AR.IntelExp.Imm;
+      if (AR.IntelExp.hasOffset())
+        OS << (AR.IntelExp.hasRegs() ? " + offset " : "offset ")
+           << AR.IntelExp.OffsetName;
+      if (AR.IntelExp.Imm || AR.IntelExp.emitImm())
+        OS << (AR.IntelExp.emitImm() ? "$$" : " + $$") << AR.IntelExp.Imm;
       if (AR.IntelExp.NeedBracs)
         OS << "]";
       break;
     case AOK_Label:
       OS << Ctx.getAsmInfo()->getPrivateLabelPrefix() << AR.Label;
       break;
     case AOK_Input:
       OS << '$' << InputIdx++;

lib/Target/X86/AsmParser/X86AsmParser.cpp

enum InfixCalculatorTok {
     IC_REGISTER
   };
 
   enum IntelOperatorKind {
     IOK_INVALID = 0,
     IOK_LENGTH,
     IOK_SIZE,
     IOK_TYPE,
-    IOK_OFFSET
   };
 
   class InfixCalculator {
     typedef std::pair< InfixCalculatorTok, int64_t > ICToken;
     SmallVector<InfixCalculatorTok, 4> InfixOperatorStack;
     SmallVector<ICToken, 4> PostfixStack;
 
     bool isUnaryOperator(const InfixCalculatorTok Op) {
enum IntelExprState {
     IES_INIT,
     IES_OR,
     IES_XOR,
     IES_AND,
     IES_LSHIFT,
     IES_RSHIFT,
     IES_PLUS,
     IES_MINUS,
+    IES_OFFSET,
     IES_NOT,
     IES_MULTIPLY,
     IES_DIVIDE,
     IES_MOD,
     IES_LBRAC,
     IES_RBRAC,
     IES_LPAREN,
     IES_RPAREN,
     IES_REGISTER,
     IES_INTEGER,
     IES_IDENTIFIER,
     IES_ERROR
   };
 
   class IntelExprStateMachine {
     IntelExprState State, PrevState;
     unsigned BaseReg, IndexReg, TmpReg, Scale;
     int64_t Imm;
     const MCExpr *Sym;
     StringRef SymName;
     InfixCalculator IC;
     InlineAsmIdentifierInfo Info;
     short BracCount;
     bool MemExpr;
+    bool OffsetOperator;
+
+    bool setSymRef(const MCExpr *Val, StringRef ID, StringRef &ErrMsg) {
+      if (Sym) {
+        ErrMsg = "cannot use more than one symbol in memory operand";
+        return true;
+      }
+      Sym = Val;
+      SymName = ID;
+      return false;
+    }
 
   public:
     IntelExprStateMachine()
         : State(IES_INIT), PrevState(IES_ERROR), BaseReg(0), IndexReg(0),
           TmpReg(0), Scale(1), Imm(0), Sym(nullptr), BracCount(0),
-          MemExpr(false) {}
+          MemExpr(false), OffsetOperator(false) {}
 
     void addImm(int64_t imm) { Imm += imm; }
     short getBracCount() { return BracCount; }
     bool isMemExpr() { return MemExpr; }
+    bool isOffsetOperator() { return OffsetOperator; }
     unsigned getBaseReg() { return BaseReg; }
     unsigned getIndexReg() { return IndexReg; }
     unsigned getScale() { return Scale; }
     const MCExpr *getSym() { return Sym; }
     StringRef getSymName() { return SymName; }
     int64_t getImm() { return Imm + IC.execute(); }
     bool isValidEndState() {
       return State == IES_RBRAC || State == IES_INTEGER;
bool onPlus(StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_INTEGER:
       case IES_RPAREN:
       case IES_REGISTER:
+      case IES_OFFSET:
         State = IES_PLUS;
         IC.pushOperator(IC_PLUS);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
           // If we already have a BaseReg, then assume this is the IndexReg with
           // a scale of 1.
           if (!BaseReg) {
             BaseReg = TmpReg;
           } else {
bool onMinus(StringRef &ErrMsg) {
       case IES_MOD:
       case IES_LPAREN:
       case IES_RPAREN:
       case IES_LBRAC:
       case IES_RBRAC:
       case IES_INTEGER:
       case IES_REGISTER:
       case IES_INIT:
+      case IES_OFFSET:
         State = IES_MINUS;
         // push minus operator if it is not a negate operator
         if (CurrState == IES_REGISTER || CurrState == IES_RPAREN ||
-            CurrState == IES_INTEGER  || CurrState == IES_RBRAC)
+            CurrState == IES_INTEGER  || CurrState == IES_RBRAC  ||
+            CurrState == IES_OFFSET)
           IC.pushOperator(IC_MINUS);
         else if (PrevState == IES_REGISTER && CurrState == IES_MULTIPLY) {
           // We have negate operator for Scale: it's illegal
           ErrMsg = "Scale can't be negative";
           return true;
         } else
           IC.pushOperator(IC_NEG);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
void onNot() {
       case IES_LBRAC:
       case IES_INIT:
         State = IES_NOT;
         IC.pushOperator(IC_NOT);
         break;
       }
       PrevState = CurrState;
     }
-
     bool onRegister(unsigned Reg, StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
       case IES_LPAREN:
bool onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName,
       // InlineAsm: Treat an enum value as an integer
       if (ParsingInlineAsm)
         if (IDInfo.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
           return onInteger(IDInfo.Enum.EnumVal, ErrMsg);
       // Treat a symbolic constant like an integer
       if (auto *CE = dyn_cast<MCConstantExpr>(SymRef))
         return onInteger(CE->getValue(), ErrMsg);
       PrevState = State;
-      bool HasSymbol = Sym != nullptr;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
       case IES_MINUS:
       case IES_NOT:
       case IES_INIT:
       case IES_LBRAC:
+        if (setSymRef(SymRef, SymRefName, ErrMsg))
+          return true;
         MemExpr = true;
         State = IES_INTEGER;
-        Sym = SymRef;
-        SymName = SymRefName;
         IC.pushOperand(IC_IMM);
         if (ParsingInlineAsm)
           Info = IDInfo;
         break;
       }
-      if (HasSymbol)
-        ErrMsg = "cannot use more than one symbol in memory operand";
-      return HasSymbol;
+      return false;
     }
     bool onInteger(int64_t TmpInt, StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
void onRParen() {
       case IES_INTEGER:
       case IES_REGISTER:
       case IES_RPAREN:
         State = IES_RPAREN;
         IC.pushOperator(IC_RPAREN);
         break;
       }
     }
+    bool onOffset(const MCExpr *Val, StringRef ID, StringRef &ErrMsg) {
+      PrevState = State;
+      switch (State) {
+      default:
+        ErrMsg = "unexpected offset operator expression";
+        return true;
+      case IES_PLUS:
+      case IES_INIT:
+      case IES_LBRAC:
+        if (setSymRef(Val, ID, ErrMsg))
+          return true;
+        OffsetOperator = true;
+        State = IES_OFFSET;
+        // As we cannot yet resolve the actual value (offset), we retain
+        // the requested semantics by pushing a '0' to the operands stack
+        IC.pushOperand(IC_IMM);
+        break;
+      }
+      return false;
+    }
   };
 
   bool Error(SMLoc L, const Twine &Msg, SMRange Range = None,
              bool MatchingInlineAsm = false) {
     MCAsmParser &Parser = getParser();
     if (MatchingInlineAsm) {
       if (!getLexer().isAtStartOfStatement())
         Parser.eatToEndOfStatement();
private:
   AddDefaultSrcDestOperands(OperandVector &Operands,
                             std::unique_ptr<llvm::MCParsedAsmOperand> &&Src,
                             std::unique_ptr<llvm::MCParsedAsmOperand> &&Dst);
   bool VerifyAndAdjustOperands(OperandVector &OrigOperands,
                                OperandVector &FinalOperands);
   std::unique_ptr<X86Operand> ParseOperand();
   std::unique_ptr<X86Operand> ParseATTOperand();
   std::unique_ptr<X86Operand> ParseIntelOperand();
-  std::unique_ptr<X86Operand> ParseIntelOffsetOfOperator();
+  bool ParseIntelOffsetOperator(const MCExpr *&Val, StringRef &ID, SMLoc &End);
   bool ParseIntelDotOperator(IntelExprStateMachine &SM, SMLoc &End);
   unsigned IdentifyIntelInlineAsmOperator(StringRef Name);
   unsigned ParseIntelInlineAsmOperator(unsigned OpKind);
   std::unique_ptr<X86Operand> ParseRoundingModeOp(SMLoc Start, SMLoc End);
-  bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM);
+  bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM,
+                               bool &ParseError, SMLoc &End);
   void RewriteIntelExpression(IntelExprStateMachine &SM, SMLoc Start,
                               SMLoc End);
   bool ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End);
   bool ParseIntelInlineAsmIdentifier(const MCExpr *&Val, StringRef &Identifier,
                                      InlineAsmIdentifierInfo &Info,
-                                     bool IsUnevaluatedOperand, SMLoc &End);
+                                     bool IsUnevaluatedOperand, SMLoc &End,
+                                     bool IsParsingOffsetOperator = false);
 
   std::unique_ptr<X86Operand> ParseMemOperand(unsigned SegReg, SMLoc StartLoc);
 
   bool ParseIntelMemoryOperandSize(unsigned &Size);
   std::unique_ptr<X86Operand>
   CreateMemForInlineAsm(unsigned SegReg, const MCExpr *Disp, unsigned BaseReg,
                         unsigned IndexReg, unsigned Scale, SMLoc Start,
                         SMLoc End, unsigned Size, StringRef Identifier,
return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
                                  IndexReg, Scale, Start, End, Size, Identifier,
                                  Decl, FrontendSize);
   }
 }
 
 // Some binary bitwise operators have a named synonymous
 // Query a candidate string for being such a named operator
 // and if so - invoke the appropriate handler
-bool X86AsmParser::ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM) {
+bool X86AsmParser::ParseIntelNamedOperator(StringRef Name,
+                                           IntelExprStateMachine &SM,
+                                           bool &ParseError, SMLoc &End) {
   // A named operator should be either lower or upper case, but not a mix
   if (Name.compare(Name.lower()) && Name.compare(Name.upper()))
     return false;
-  if (Name.equals_lower("not"))
+  if (Name.equals_lower("not")) {
     SM.onNot();
-  else if (Name.equals_lower("or"))
+  } else if (Name.equals_lower("or")) {
     SM.onOr();
-  else if (Name.equals_lower("shl"))
+  } else if (Name.equals_lower("shl")) {
     SM.onLShift();
-  else if (Name.equals_lower("shr"))
+  } else if (Name.equals_lower("shr")) {
     SM.onRShift();
-  else if (Name.equals_lower("xor"))
+  } else if (Name.equals_lower("xor")) {
     SM.onXor();
-  else if (Name.equals_lower("and"))
+  } else if (Name.equals_lower("and")) {
     SM.onAnd();
-  else if (Name.equals_lower("mod"))
+  } else if (Name.equals_lower("mod")) {
     SM.onMod();
-  else
+  } else if (Name.equals_lower("offset")) {
+    const MCExpr *Val = nullptr;
+    StringRef ID;
+    if ((ParseError = ParseIntelOffsetOperator(Val, ID, End)))
+      return true;
+    StringRef ErrMsg;
+    if ((ParseError = SM.onOffset(Val, ID, ErrMsg)))
+      return Error(SMLoc::getFromPointer(Name.data()), ErrMsg);
+  } else {
     return false;
+  }
+  if (!Name.equals_lower("offset"))
+    End = consumeToken();
   return true;
 }
 
 bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
   StringRef ErrMsg;
 
case AsmToken::Identifier: {
       // Register
       unsigned Reg;
       if (Tok.isNot(AsmToken::String) && !ParseRegister(Reg, IdentLoc, End)) {
         if (SM.onRegister(Reg, ErrMsg))
           return Error(Tok.getLoc(), ErrMsg);
         break;
       }
       // Operator synonymous ("not", "or" etc.)
-      if ((UpdateLocLex = ParseIntelNamedOperator(Identifier, SM)))
+      bool ParseError = false;
+      if (ParseIntelNamedOperator(Identifier, SM, ParseError, End)) {
+        if (ParseError)
+          return true;
         break;
+      }
       // Symbol reference, when parsing assembly content
       InlineAsmIdentifierInfo Info;
       const MCExpr *Val;
       if (!isParsingInlineAsm()) {
         if (getParser().parsePrimaryExpr(Val, End)) {
           return Error(Tok.getLoc(), "Unexpected identifier!");
         } else if (SM.onIdentifierExpr(Val, Identifier, Info, false, ErrMsg)) {
           return Error(IdentLoc, ErrMsg);
         } else
           break;
       }
       // MS InlineAsm operators (TYPE/LENGTH/SIZE)
       if (unsigned OpKind = IdentifyIntelInlineAsmOperator(Identifier)) {
-        if (OpKind == IOK_OFFSET)
-          return Error(IdentLoc, "Dealing OFFSET operator as part of"
-            "a compound immediate expression is yet to be supported");
         if (int64_t Val = ParseIntelInlineAsmOperator(OpKind)) {
           if (SM.onInteger(Val, ErrMsg))
             return Error(IdentLoc, ErrMsg);
         } else
           return true;
         break;
       }
       // MS Dot Operator expression
bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
   return false;
 }
 
 void X86AsmParser::RewriteIntelExpression(IntelExprStateMachine &SM,
                                           SMLoc Start, SMLoc End) {
   SMLoc Loc = Start;
   unsigned ExprLen = End.getPointer() - Start.getPointer();
   // Skip everything before a symbol displacement (if we have one)
-  if (SM.getSym()) {
+  if (SM.getSym() && !SM.isOffsetOperator()) {
     StringRef SymName = SM.getSymName();
-    if (unsigned Len =  SymName.data() - Start.getPointer())
+    if (unsigned Len = SymName.data() - Start.getPointer())
       InstInfo->AsmRewrites->emplace_back(AOK_Skip, Start, Len);
     Loc = SMLoc::getFromPointer(SymName.data() + SymName.size());
     ExprLen = End.getPointer() - (SymName.data() + SymName.size());
     // If we have only a symbol than there's no need for complex rewrite,
     // simply skip everything after it
     if (!(SM.getBaseReg() || SM.getIndexReg() || SM.getImm())) {
       if (ExprLen)
         InstInfo->AsmRewrites->emplace_back(AOK_Skip, Loc, ExprLen);
       return;
     }
   }
   // Build an Intel Expression rewrite
   StringRef BaseRegStr;
   StringRef IndexRegStr;
+  StringRef OffsetNameStr;
   if (SM.getBaseReg())
     BaseRegStr = X86IntelInstPrinter::getRegisterName(SM.getBaseReg());
   if (SM.getIndexReg())
     IndexRegStr = X86IntelInstPrinter::getRegisterName(SM.getIndexReg());
+  if (SM.isOffsetOperator())
+    OffsetNameStr = SM.getSymName();
   // Emit it
-  IntelExpr Expr(BaseRegStr, IndexRegStr, SM.getScale(), SM.getImm(), SM.isMemExpr());
+  IntelExpr Expr(BaseRegStr, IndexRegStr, SM.getScale(), OffsetNameStr,
+                 SM.getImm(), SM.isMemExpr());
   InstInfo->AsmRewrites->emplace_back(Loc, ExprLen, Expr);
 }
 
 // Inline assembly may use variable names with namespace alias qualifiers.
-bool X86AsmParser::ParseIntelInlineAsmIdentifier(const MCExpr *&Val,
-                                                 StringRef &Identifier,
-                                                 InlineAsmIdentifierInfo &Info,
-                                                 bool IsUnevaluatedOperand,
-                                                 SMLoc &End) {
+bool
+X86AsmParser::ParseIntelInlineAsmIdentifier(const MCExpr *&Val,
+                                            SMLoc &End,
+                                            bool IsParsingOffsetOperator) {
   MCAsmParser &Parser = getParser();
   assert(isParsingInlineAsm() && "Expected to be parsing inline assembly.");
   Val = nullptr;
 
   StringRef LineBuf(Identifier.data());
   SemaCallback->LookupInlineAsmIdentifier(LineBuf, Info, IsUnevaluatedOperand);
 
   const AsmToken &Tok = Parser.getTok();
X86AsmParser::ParseIntelInlineAsmIdentifier(const MCExpr *&Val,
 
   // If the identifier lookup was unsuccessful, assume that we are dealing with
   // a label.
   if (Info.isKind(InlineAsmIdentifierInfo::IK_Invalid)) {
     StringRef InternalName =
       SemaCallback->LookupInlineAsmLabel(Identifier, getSourceManager(),
                                          Loc, false);
     assert(InternalName.size() && "We should have an internal name here.");
-    // Push a rewrite for replacing the identifier name with the internal name.
+    // Push a rewrite for replacing the identifier name with the internal name,
+    // unless we are parsing the operand of an offset operator
+    if (!IsParsingOffsetOperator)
       InstInfo->AsmRewrites->emplace_back(AOK_Label, Loc, Identifier.size(),
                                           InternalName);
+    else
+      Identifier = InternalName;
   } else if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
     return false;
   // Create the symbol reference.
   MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
   MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
   Val = MCSymbolRefExpr::create(Sym, Variant, getParser().getContext());
   return false;
 }
bool X86AsmParser::ParseIntelDotOperator(IntelExprStateMachine &SM, SMLoc &End) {
   End = SMLoc::getFromPointer(DotDispStr.data());
   const char *DotExprEndLoc = DotDispStr.data() + DotDispStr.size();
   while (Tok.getLoc().getPointer() < DotExprEndLoc)
     Lex();
   SM.addImm(Offset);
   return false;
 }
 
-/// Parse the 'offset' operator.  This operator is used to specify the
-/// location rather then the content of a variable.
-std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOffsetOfOperator() {
-  MCAsmParser &Parser = getParser();
-  const AsmToken &Tok = Parser.getTok();
-  SMLoc OffsetOfLoc = Tok.getLoc();
-  Parser.Lex(); // Eat offset.
-
-  const MCExpr *Val;
+/// Parse the 'offset' operator.
+/// This operator is used to specify the location of a given operand
+bool X86AsmParser::ParseIntelOffsetOperator(const MCExpr *&Val, StringRef &ID,
+                                            SMLoc &End) {
+  // Eat offset, mark start of identifier.
+  SMLoc Start = Lex().getLoc();
+  ID = getTok().getString();
   InlineAsmIdentifierInfo Info;
-  SMLoc Start = Tok.getLoc(), End;
-  StringRef Identifier = Tok.getString();
-  if (ParseIntelInlineAsmIdentifier(Val, Identifier, Info,
-                                    /*Unevaluated=*/false, End))
-    return nullptr;
-
-  void *Decl = nullptr;
-  // FIXME: MS evaluates "offset <Constant>" to the underlying integral
-  if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal))
-    return ErrorOperand(Start, "offset operator cannot yet handle constants");
-  else if (Info.isKind(InlineAsmIdentifierInfo::IK_Var))
-    Decl = Info.Var.Decl;
-  // Don't emit the offset operator.
-  InstInfo->AsmRewrites->emplace_back(AOK_Skip, OffsetOfLoc, 7);
-
-  // The offset operator will have an 'r' constraint, thus we need to create
-  // register operand to ensure proper matching.  Just pick a GPR based on
-  // the size of a pointer.
-  bool Parse32 = is32BitMode() || Code16GCC;
-  unsigned RegNo = is64BitMode() ? X86::RBX : (Parse32 ? X86::EBX : X86::BX);
-
-  return X86Operand::CreateReg(RegNo, Start, End, /*GetAddress=*/true,
-                               OffsetOfLoc, Identifier, Decl);
+  if (!isParsingInlineAsm()) {
+    if (getTok().isNot(AsmToken::Identifier) ||
+        getParser().parsePrimaryExpr(Val, End))
+      return Error(Start, "unexpected token!");
+  } else if (ParseIntelInlineAsmIdentifier(Val, ID, Info, false, End, true)) {
+    return Error(Start, "unable to lookup expression");
+  } else if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal)) {
+    return Error(Start, "offset operator cannot yet handle constants");
+  } else if (Info.isKind(InlineAsmIdentifierInfo::IK_Var) &&
+             !Info.Var.IsGlobalLV) {
+    return Error(SMLoc::getFromPointer(ID.data()), "illegal operand for "
+                                                   "offset operator");
+  }
+  return false;
 }
 
 // Query a candidate string for being an Intel assembly operator
 // Report back its kind, or IOK_INVALID if does not evaluated as a known one
 unsigned X86AsmParser::IdentifyIntelInlineAsmOperator(StringRef Name) {
   return StringSwitch<unsigned>(Name)
     .Cases("TYPE","type",IOK_TYPE)
     .Cases("SIZE","size",IOK_SIZE)
     .Cases("LENGTH","length",IOK_LENGTH)
-    .Cases("OFFSET","offset",IOK_OFFSET)
     .Default(IOK_INVALID);
 }
 
 /// Parse the 'LENGTH', 'TYPE' and 'SIZE' operators.  The LENGTH operator
 /// returns the number of elements in an array.  It returns the value 1 for
 /// non-array variables.  The SIZE operator returns the size of a C or C++
 /// variable.  A variable's size is the product of its LENGTH and TYPE.  The
 /// TYPE operator returns the size of a C or C++ type or variable. If the
bool X86AsmParser::ParseIntelMemoryOperandSize(unsigned &Size) {
   return false;
 }
 
 std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOperand() {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
   SMLoc Start, End;
 
-  // FIXME: Offset operator
-  // Should be handled as part of immediate expression, as other operators
-  // Currently, only supported as a stand-alone operand
-  if (isParsingInlineAsm())
-    if (IdentifyIntelInlineAsmOperator(Tok.getString()) == IOK_OFFSET)
-      return ParseIntelOffsetOfOperator();
-
   // Parse optional Size directive.
   unsigned Size;
   if (ParseIntelMemoryOperandSize(Size))
     return nullptr;
   bool PtrInOperand = bool(Size);
 
   Start = Tok.getLoc();
 

test/CodeGen/X86/offset-operator.ll

+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -x86-asm-syntax=intel -relocation-model=static < %s | FileCheck %s
+
+; Test we are emitting the 'offset' operator upon an immediate reference of a label:
+; The emitted 'att-equivalent' of this one is "movl $.L.str, %eax"
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+
+define i8* @test_offset_operator() {
+; CHECK-LABEL: test_offset_operator:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    mov eax, offset .L.str
+; CHECK-NEXT:    ret
+entry:
+  ret i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0)
+}

test/MC/X86/pr32530.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s
+
+.text
+// CHECK: movq    $msg, %rsi
+// CHECK: movq    $msg+314159, %rax
+// CHECK: movq    $msg-89793, msg-6535(%rax,%rbx,2)
+  mov rsi,  offset msg
+  mov rax,  offset msg + 314159
+  mov qword ptr [rax + 2*rbx + offset msg - 6535],  offset msg - 89793
+.data
+msg:
+  .ascii "Hello, world!\n"
+