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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei 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]
----------------
xiangzhangllvm wrote:
> 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)
As you showed, the `P` is used for call instruction. I don't think it's a good idea to apply it to non call instruction.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5990
+      return false;
+  }
+
----------------
xiangzhangllvm wrote:
> 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.
I think you misunderstood efriedma's point. He meant you should distinguish during parsing instead of distinguishing again.
Besides, I think you missed the case when ARR inside [], e.g, `[gVar + ecx + ebx]`. I agreed with him the logic is magic and not easy to understand.
D121072 intends do the mangling at front end. No following process is needed. I see front end do mangling sometime, so I think we can do it there.


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

https://reviews.llvm.org/D120887