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

[Xiang Zhang via Phabricator via cfe-commits]

xiangzhangllvm added inline comments.


================
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)) {
----------------
skan wrote:
> 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.
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 ?


================
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();
----------------
skan wrote:
> 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.
> 
> RIP and IndexReg can never be used together according to design of X86 instruction. 
Good answer!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113096