[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]
----------------
pengfei wrote:
> 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.
We can modify the description in  asm-template-argument-modifiers , because the real implement of it is only print non-RIP in X86AsmPrinter. Now reason show non-rip address can on work on function label. That is not a beautiful design from beginning.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5990
+      return false;
+  }
+
----------------
pengfei wrote:
> 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.
> D121072 intends do the mangling at front end. No following process is needed. 
We can't make sure "No following process is needed. "
One problem I can see is a symbol both used for lea and call. Call may @plt (or some others), but lea not. 

In fact, D121072 is only the idea of do part print job at codegen. If it can be replaced in MIR why it can be directly repalced in IR ? Just try getSymbol (which will use mangler name)
That is not a good arch for IR/MIR. All the operands should be visible to IR/MIR.




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

https://reviews.llvm.org/D120887