[PATCH] D120887: The [2/2] Fix mangle problem when variable used in inline asm （non-rip for ARR[BaseReg+IndexReg+..]）

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: clang/test/CodeGen/ms-inline-asm-variables.c:14
   __asm add dword ptr [ebx + gVar + 828], ebx
-  // CHECK: add ecx, dword ptr ${{[0-9]}}[ecx + ecx * $$4 + $$4590]
+  // CHECK: add ecx, dword ptr ${{{[0-9]}}:P}[ecx + ecx * $$4 + $$4590]
   __asm add ecx, dword ptr gVar[4590 + ecx + ecx*4]
----------------
LuoYuanke wrote:
> LuoYuanke wrote:
> > Is the ":P" you added fit the below description from https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers?
> > 
> > `P: Print a memory reference or operand for use as the argument of a call instruction. (E.g. omit (rip), even though it’s PC-relative.)`
> > 
> > Should the ":P" be specified only in -pic mode?
> Would you post the example code lowering from IR to assembly? I can't create an valid IR code with `:P` flag in https://godbolt.org/z/doP83Wr1a
Yes, it fit  [[ https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers | asm-template-argument-modifiers]] , 

```
P: Print a memory reference or operand for use as the argument of a call instruction. (E.g. omit (rip), even though it’s PC-relative.)
```
In fact, the real implement of "P" is only non-RIP in  X86AsmPrinter


================
Comment at: clang/test/CodeGen/ms-inline-asm-variables.c:14
   __asm add dword ptr [ebx + gVar + 828], ebx
-  // CHECK: add ecx, dword ptr ${{[0-9]}}[ecx + ecx * $$4 + $$4590]
+  // CHECK: add ecx, dword ptr ${{{[0-9]}}:P}[ecx + ecx * $$4 + $$4590]
   __asm add ecx, dword ptr gVar[4590 + ecx + ecx*4]
----------------
xiangzhangllvm wrote:
> LuoYuanke wrote:
> > LuoYuanke wrote:
> > > Is the ":P" you added fit the below description from https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers?
> > > 
> > > `P: Print a memory reference or operand for use as the argument of a call instruction. (E.g. omit (rip), even though it’s PC-relative.)`
> > > 
> > > Should the ":P" be specified only in -pic mode?
> > Would you post the example code lowering from IR to assembly? I can't create an valid IR code with `:P` flag in https://godbolt.org/z/doP83Wr1a
> Yes, it fit  [[ https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers | asm-template-argument-modifiers]] , 
> 
> ```
> P: Print a memory reference or operand for use as the argument of a call instruction. (E.g. omit (rip), even though it’s PC-relative.)
> ```
> In fact, the real implement of "P" is only non-RIP in  X86AsmPrinter
Try use llc to build it. The fail in https://godbolt.org/z/doP83Wr1a is not relation with "P"
(I can't logon my sever now)


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5990
+      return false;
+  }
+
----------------
pengfei wrote:
> xiangzhangllvm wrote:
> > efriedma wrote:
> > > If I'm understanding correctly, this is trying to detect two adjacent expressions in a specific form, and then transform one of them so they can be concatenated?  This is way too much magic.  If we aren't parsing expressions correctly, please fix the initial parse; don't fudge the expressions afterwards.
> > I think you understand a part of it.
> > Here the instruction parsing are finished. The Operands info written into AsmStrRewrites[].
> > Let me take a example:
> > Both follow cases will get same Operands info into AsmStrRewrites[i] (ARR) and AsmStrRewrites[i+1] ([edx+eax*4])
> > Case 1: 
> > ```
> >   mov     eax,ARR[edx+eax*4]
> > ```
> > Case 2:
> > ```
> >  mov     eax,ARR ;   call  dword ptr[edx+eax*4]
> > ```
> > So here code (line 5967-5990) is just to distinguish the 2 case.
> > 
> > For Case 1, we can not let ARR be a RIP related address, because it already has base reg and index reg. (We can't use 3 regs for a mem address)
> > For Case 2, it is no problem let ARR use RIP, the [edx+eax*4] are in another instruction.
> I agree with @efriedma and think the use of `:P` is hacky. I wrote an experiment patch which seems more concise D121072.
I have explained that efriedma's understand is not correct. There is not **concatenated **them, just distinguish. PLS refer my previous explain.
> and then transform one of them **so they can be concatenated**? This is way too much magic. If we ...

D121072 is still try to replace the Global name in IR/MIR, that is not good way to let follow process see it. (for example Mangler), and no reason do it on codegen. Using Modifer ":P", let all process see it, and let exsiting code to handle it, in my eye, is better.


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

https://reviews.llvm.org/D120887