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

[Kan Shengchen via Phabricator via cfe-commits]

skan marked 3 inline comments as done.
skan added inline comments.


================
Comment at: clang/test/CodeGen/ms-inline-asm-static-variable.c:8
+  // 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
----------------
pengfei wrote:
> skan wrote:
> > pengfei wrote:
> > > IIRC, we have limitation on the number. Can you check if it works when we have more than 10 global variables.
> > sorry, which number?
> `$0`. I mean if we have arr1, arr2, ..., we should have `$1`, `$2`, ... but the max number is `$9` if I remember it correctly.
We does not touch that part of the code. If there was this limitation before, there is now.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1759
   // 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)) {
----------------
xiangzhangllvm wrote:
> 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 ?
> 
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.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2554-2555
   unsigned IndexReg = SM.getIndexReg();
+  if (IndexReg && BaseReg == X86::RIP)
+    BaseReg = 0;
   unsigned Scale = SM.getScale();
----------------
xiangzhangllvm wrote:
> 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?
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113096/new/

https://reviews.llvm.org/D113096