[X86][MS-InlineAsm] Add constraint *m for memory access w/ global var

[skan]

Summary

Constraint *m should be used when the address of a variable is passed
as a value. And the constraint is missing for MS inline assembly when sth
is written to the address of the variable.

The missing would cause FE delete the definition of the static varible,
and then result in "undefined reference to xxx" issue.

[skan]

clang-format the diff

[pengfei]

How about the GV is function pointer? I believe @xiangzhangllvm has lots of experience on it :)

clang/test/CodeGen/ms-inline-asm-static-variable.c

9

IIRC, we have limitation on the number. Can you check if it works when we have more than 10 global variables.

clang/test/CodeGen/ms-inline-asm-variables.c

35

Unrelated change.

llvm/test/CodeGen/X86/ms-inline-asm-array.ll

16–23

These seem unnecessary.

[skan]

clang/test/CodeGen/ms-inline-asm-static-variable.c

9

sorry, which number?

clang/test/CodeGen/ms-inline-asm-variables.c

35

Just delete a blank line, which is done by auto clang-format. I think it's okay.

[skan]

llvm/test/CodeGen/X86/ms-inline-asm-array.ll

16–23

This is necessary b/c we need to check the inline-asm IR can be rerwritten to correct assmbly after constraint "*m" is added.

[xiangzhangllvm]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1759

Let me generally tell out my understand here, (If wrong PLS correct me)
Here from the comments we can see, the old code want to keep the origin symbol of global variable to let linker (relocation) handle it. Here you describe it with a pointer (with decl), it change to form of $ID <--> (decl), So which need constrain it with "*m". But if the pointer can not be access from BaseReg(Rip) + Index(Ip) how do you descript the pointer you generate out ?

2554–2555

The change here looks too arbitrary. For global address it is ok to drop the base, it mainly fetch from offset. but if here not global variable?

[pengfei]

clang/test/CodeGen/ms-inline-asm-static-variable.c

9

$0. I mean if we have arr1, arr2, ..., we should have $1, $2, ... but the max number is $9 if I remember it correctly.

[skan]

clang/test/CodeGen/ms-inline-asm-static-variable.c

9

We does not touch that part of the code. If there was this limitation before, there is now.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1759

I think you may misunderstand this code.

This code handles the memory that can not be represented by Disp(RIP) b/c there is already a BaseReg or IndexReg there.

Before this patch, the memory is represented like arr[edx*4] and there is no identifer bound to it. And after this patch, we bind the memory to identifer arr.

2554–2555

RIP and IndexReg can never be used together according to design of X86 instruction. The BaseReg is set to RIP b/c we add the constaint "*m" in the MS-inline assembly. So we can drop it safely.

I acknowledge it's not the best way to do it, but it's simplest. Similary, you can see the line 2560-2562. RSP can never be a IndexReg, but we just swap the BaseReg w/ IndexReg b/c it is not handled well in previous phases.

[xiangzhangllvm]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1759

Yes, that is why I mean t change ( arr[edx*4] ) to form of $ID <--> (decl). So what the problem if we let the old form ( arr[edx*4] ) being ?

2554–2555

RIP and IndexReg can never be used together according to design of X86 instruction.

Good answer!

[skan]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1759

Then the definition of arr would be deleted by C Front end, and result in "undefined reference to xxx" issue during link stage.

[thakis]

Looks like this breaks tests on Mac: http://45.33.8.238/mac/38266/step_11.txt

Please take a look and revert for now if it takes a while to fix.

[skan]

In D113096#3110507, @thakis wrote:

Looks like this breaks tests on Mac: http://45.33.8.238/mac/38266/step_11.txt

Please take a look and revert for now if it takes a while to fix.

I can quickly fix it.

It's the issue of ambiguous triple. You are using the -mtriple=x86_64-apple-mac* while I am using -mtriple=x86_64-unknown-linux-gnu

[skan]

In D113096#3110546, @skan wrote:

In D113096#3110507, @thakis wrote:

Looks like this breaks tests on Mac: http://45.33.8.238/mac/38266/step_11.txt

Please take a look and revert for now if it takes a while to fix.

I can quickly fix it.

It's the issue of ambiguous triple. You are using the -mtriple=x86_64-apple-mac* while I am using -mtriple=x86_64-unknown-linux-gnu

Fixed by 6d03227c16ee

[pengfei]

@skan This change seems don't work for -fpic, see https://godbolt.org/z/h3nWoerPe
I don't have any idea to handle these cases, I suggest we should revert this patch first.

clang/test/CodeGen/X86/ms_fmul.c

 
 void __attribute__ ((naked)) foo(void)
 {__asm{
     fmul qword ptr [static_const_table + 0x00f0 +edx]
     ret
 }}
 
 // CHECK-LABEL: foo
-// CHECK: call void asm sideeffect inteldialect "fmul qword ptr static_const_table[edx + $$240]\0A\09ret"
+// CHECK: call void asm sideeffect inteldialect "fmul qword ptr $0[edx + $$240]\0A\09ret"

clang/test/CodeGen/ms-inline-asm-static-variable.c

+// REQUIRES: x86-registered-target
+// Check the constraint "*m" of operand arr and the definition of arr is not removed by FE
+// RUN: %clang_cc1 %s -fasm-blocks -triple i386-apple-darwin10 -emit-llvm -o - | FileCheck %s
+
+static int arr[10];
+void t1() {
+  // CHECK: @arr = internal global [10 x i32]
+  // CHECK: call void asm sideeffect inteldialect "mov dword ptr $0[edx * $$4],edx", "=*m,{{.*}}([10 x i32]* @arr)
+  __asm mov  dword ptr arr[edx*4],edx
+}

clang/test/CodeGen/ms-inline-asm-variables.c

 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 %s -fasm-blocks -triple i386-apple-darwin10 -emit-llvm -o - | FileCheck %s
 
 int gVar;
 void t1() {
-  // CHECK: add eax, dword ptr gVar[eax]
+  // CHECK: add eax, dword ptr ${{[0-9]}}[eax]
   __asm add eax, dword ptr gVar[eax]
-  // CHECK: add dword ptr gVar[eax], eax
+  // CHECK: add dword ptr ${{[0-9]}}[eax], eax
   __asm add dword ptr [eax+gVar], eax
-  // CHECK: add ebx, dword ptr gVar[ebx + $$270]
+  // CHECK: add ebx, dword ptr ${{[0-9]}}[ebx + $$270]
   __asm add ebx, dword ptr gVar[271 - 82 + 81 + ebx]
-  // CHECK: add dword ptr gVar[ebx + $$828], ebx
+  // CHECK: add dword ptr ${{[0-9]}}[ebx + $$828], ebx
   __asm add dword ptr [ebx + gVar + 828], ebx
-  // CHECK: add ecx, dword ptr gVar[ecx + ecx * $$4 + $$4590]
+  // CHECK: add ecx, dword ptr ${{[0-9]}}[ecx + ecx * $$4 + $$4590]
   __asm add ecx, dword ptr gVar[4590 + ecx + ecx*4]
-  // CHECK: add dword ptr gVar[ecx + ecx * $$8 + $$73], ecx
+  // CHECK: add dword ptr ${{[0-9]}}[ecx + ecx * $$8 + $$73], ecx
   __asm add dword ptr [gVar + ecx + 45 + 23 - 53 + 60 - 2 + ecx*8], ecx
-  // CHECK: add gVar[ecx + ebx + $$7], eax
+  // CHECK: add ${{[0-9]}}[ecx + ebx + $$7], eax
   __asm add 1 + 1 + 2 + 3[gVar + ecx + ebx], eax
 }
 
 void t2() {
   int lVar;
   // CHECK: mov eax, dword ptr ${{[0-9]}}[eax]
   __asm mov eax, dword ptr lVar[eax]
   // CHECK: mov dword ptr ${{[0-9]}}[eax], eax
   __asm mov dword ptr [eax+lVar], eax
   // CHECK: mov ebx, dword ptr ${{[0-9]}}[ebx + $$270]
   __asm mov ebx, dword ptr lVar[271 - 82 + 81 + ebx]
   // CHECK: mov dword ptr ${{[0-9]}}[ebx + $$828], ebx
   __asm mov dword ptr [ebx + lVar + 828], ebx
   // CHECK: mov ${{[0-9]}}[ebx + $$47], eax
   __asm mov 5 + 8 + 13 + 21[lVar + ebx], eax
 }
-

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

bool X86AsmParser::CreateMemForMSInlineAsm(
   bool IsGlobalLV = false;
   if (Info.isKind(InlineAsmIdentifierInfo::IK_Var)) {
     // Size is in terms of bits in this context.
     FrontendSize = Info.Var.Type * 8;
     Decl = Info.Var.Decl;
     IsGlobalLV = Info.Var.IsGlobalLV;
   }
   // 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));
+    Operands.push_back(X86Operand::CreateMem(getPointerWidth(), Disp, Start,
+                                             End, Size, Identifier, Decl));
     return false;
   }
   // Otherwise, we set the base register to a non-zero value
   // if we don't know the actual value at this time.  This is necessary to
   // get the matching correct in some cases.
   BaseReg = BaseReg ? BaseReg : 1;
   Operands.push_back(X86Operand::CreateMem(
       getPointerWidth(), SegReg, Disp, BaseReg, IndexReg, Scale, Start, End,
if (!SM.isMemExpr() && !RegNo) {
 
     Operands.push_back(X86Operand::CreateImm(Disp, Start, End));
     return false;
   }
 
   StringRef ErrMsg;
   unsigned BaseReg = SM.getBaseReg();
   unsigned IndexReg = SM.getIndexReg();
+  if (IndexReg && BaseReg == X86::RIP)
+    BaseReg = 0;
   unsigned Scale = SM.getScale();
   if (!PtrInOperand)
     Size = SM.getElementSize() << 3;
 
   if (Scale == 0 && BaseReg != X86::ESP && BaseReg != X86::RSP &&
       (IndexReg == X86::ESP || IndexReg == X86::RSP))
     std::swap(BaseReg, IndexReg);
 

llvm/test/CodeGen/X86/ms-inline-asm-array.ll

+; RUN: llc < %s -mcpu=x86-64  | FileCheck %s
+
+@arr = internal global [10 x i32] zeroinitializer, align 16
+
+; CHECK: movl    %edx, arr(,%rdx,4)
+define dso_local i32 @main() #0 {
+entry:
+  call void asm sideeffect inteldialect "mov dword ptr $0[rdx * $$4],edx", "=*m,~{dirflag},~{fpsr},~{flags}"([10 x i32]* @arr) #1, !srcloc !4
+  ret i32 0
+}
+
+attributes #0 = { noinline nounwind optnone uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { nounwind }
+
+!llvm.module.flags = !{!0, !1, !2}
+!llvm.ident = !{!3}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 7, !"uwtable", i32 1}
+!2 = !{i32 7, !"frame-pointer", i32 2}
+!3 = !{!"clang"}
+!4 = !{i64 63}