ms inline asm: Fix {call,jmp} fptr

[MaskRay]

https://reviews.llvm.org/D151863 (2023-05) removed
BaseReg = BaseReg ? BaseReg : 1 (introduced in commit
175d0ae (2013)) and caused a
regression: ensuring a non-zero BaseReg was to treat
static void (*fptr)(); __asm { call fptr } as non-AbsMem
AsmOperandClass and generate call $0/callq *fptr(%rip) instead of
call ${0:P}/callq *fptr
(The asm template argument modifier P is for call targets, while
no modifier is used by other instructions like mov rax, fptr)

This patch reinstates the BaseReg-setting statement but places it inside
if (IsGlobalLV) for clarify.

The special case is unfortunate, but we also have special case in
similar places (https://reviews.llvm.org/D149920).

Fix: #73033

[llvmbot]

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

https://reviews.llvm.org/D151863 (2023-05) removed
BaseReg = BaseReg ? BaseReg : 1 (introduced in commit
175d0ae (2013)) and caused a
regression: ensuring a non-zero BaseReg was to treat
static void (*fptr)(); __asm { call fptr } as non-AbsMem
AsmOperandClass and generate call $0/callq *fptr(%rip) instead of
call ${0:P}/callq *fptr
(The asm template argument modifier P is for call targets, while
no modifier is used by other instructions like mov rax, fptr)

This patch reinstates the BaseReg-setting statement but places it inside
if (IsGlobalLV) for clarify.

The special case is unfortunate, but we also have special case in
similar places (https://reviews.llvm.org/D149920).

---

Full diff: https://github.com/llvm/llvm-project/pull/73207.diff

2 Files Affected:

• (modified) clang/test/CodeGen/ms-inline-asm-64.c (+6-1)
• (modified) llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp (+23-15)

diff --git a/clang/test/CodeGen/ms-inline-asm-64.c b/clang/test/CodeGen/ms-inline-asm-64.c
index 313d380e121bce0..c7e4c1b603bd76c 100644
--- a/clang/test/CodeGen/ms-inline-asm-64.c
+++ b/clang/test/CodeGen/ms-inline-asm-64.c
@@ -60,17 +60,22 @@ int t4(void) {
 }
 
 void bar() {}
+static void (*fptr)();
 
 void t5(void) {
   __asm {
     call bar
     jmp bar
+    call fptr
+    jmp fptr
   }
   // CHECK: t5
   // CHECK: call void asm sideeffect inteldialect
   // CHECK-SAME: call ${0:P}
   // CHECK-SAME: jmp ${1:P}
-  // CHECK-SAME: "*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(void (...)) @bar, ptr elementtype(void (...)) @bar)
+  // CHECK-SAME: call $2
+  // CHECK-SAME: jmp $3
+  // CHECK-SAME: "*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(void (...)) @bar, ptr elementtype(void (...)) @bar, ptr elementtype(ptr) @fptr, ptr elementtype(ptr) @fptr)
 }
 
 void t47(void) {
diff --git a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
index 008075163b90a8d..a02978c64412cf7 100644
--- a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -1144,8 +1144,8 @@ class X86AsmParser : public MCTargetAsmParser {
   bool ParseIntelMemoryOperandSize(unsigned &Size);
   bool CreateMemForMSInlineAsm(unsigned SegReg, const MCExpr *Disp,
                                unsigned BaseReg, unsigned IndexReg,
-                               unsigned Scale, SMLoc Start, SMLoc End,
-                               unsigned Size, StringRef Identifier,
+                               unsigned Scale, bool NonAbsMem, SMLoc Start,
+                               SMLoc End, unsigned Size, StringRef Identifier,
                                const InlineAsmIdentifierInfo &Info,
                                OperandVector &Operands);
 
@@ -1745,10 +1745,13 @@ bool X86AsmParser::parseOperand(OperandVector &Operands, StringRef Name) {
   return parseATTOperand(Operands);
 }
 
-bool X86AsmParser::CreateMemForMSInlineAsm(
-    unsigned SegReg, const MCExpr *Disp, unsigned BaseReg, unsigned IndexReg,
-    unsigned Scale, SMLoc Start, SMLoc End, unsigned Size, StringRef Identifier,
-    const InlineAsmIdentifierInfo &Info, OperandVector &Operands) {
+bool X86AsmParser::CreateMemForMSInlineAsm(unsigned SegReg, const MCExpr *Disp,
+                                           unsigned BaseReg, unsigned IndexReg,
+                                           unsigned Scale, bool NonAbsMem,
+                                           SMLoc Start, SMLoc End,
+                                           unsigned Size, StringRef Identifier,
+                                           const InlineAsmIdentifierInfo &Info,
+                                           OperandVector &Operands) {
   // If we found a decl other than a VarDecl, then assume it is a FuncDecl or
   // some other label reference.
   if (Info.isKind(InlineAsmIdentifierInfo::IK_Label)) {
@@ -1773,11 +1776,15 @@ bool X86AsmParser::CreateMemForMSInlineAsm(
   }
   // It is widely common for MS InlineAsm to use a global variable and one/two
   // registers in a mmory expression, and though unaccessible via rip/eip.
-  if (IsGlobalLV && (BaseReg || IndexReg)) {
-    Operands.push_back(X86Operand::CreateMem(getPointerWidth(), Disp, Start,
-                                             End, Size, Identifier, Decl, 0,
-                                             BaseReg && IndexReg));
-    return false;
+  if (IsGlobalLV) {
+    if (BaseReg || IndexReg) {
+      Operands.push_back(X86Operand::CreateMem(getPointerWidth(), Disp, Start,
+                                               End, Size, Identifier, Decl, 0,
+                                               BaseReg && IndexReg));
+      return false;
+    }
+    if (NonAbsMem)
+      BaseReg = 1; // Make isAbsMem() false
   }
   Operands.push_back(X86Operand::CreateMem(
       getPointerWidth(), SegReg, Disp, BaseReg, IndexReg, Scale, Start, End,
@@ -2621,9 +2628,12 @@ bool X86AsmParser::parseIntelOperand(OperandVector &Operands, StringRef Name) {
       CheckBaseRegAndIndexRegAndScale(BaseReg, IndexReg, Scale, is64BitMode(),
                                       ErrMsg))
     return Error(Start, ErrMsg);
+  bool IsUnconditionalBranch =
+      Name.equals_insensitive("jmp") || Name.equals_insensitive("call");
   if (isParsingMSInlineAsm())
-    return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale, Start,
-                                   End, Size, SM.getSymName(),
+    return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale,
+                                   IsUnconditionalBranch && is64BitMode(),
+                                   Start, End, Size, SM.getSymName(),
                                    SM.getIdentifierInfo(), Operands);
 
   // When parsing x64 MS-style assembly, all non-absolute references to a named
@@ -2631,8 +2641,6 @@ bool X86AsmParser::parseIntelOperand(OperandVector &Operands, StringRef Name) {
   unsigned DefaultBaseReg = X86::NoRegister;
   bool MaybeDirectBranchDest = true;
 
-  bool IsUnconditionalBranch =
-      Name.equals_insensitive("jmp") || Name.equals_insensitive("call");
   if (Parser.isParsingMasm()) {
     if (is64BitMode() && SM.getElementSize() > 0) {
       DefaultBaseReg = X86::RIP;

[MaskRay]

I should confess that I don't understand the mechanism well. I've tried hard to write a good description but I cannot improve the comments in CreateMemForMSInlineAsm.

[KanRobert]

Investigating, I almost forgot the mechanism of MS inline asm support...

[KanRobert]

LGTM