[PATCH] D121785: The [3/3] Fix mangle problem when variable used in inline asm （Support ARR[BaseReg+IndexReg+..] in PIC model）

Tue Mar 22 18:24:30 PDT 2022

xiangzhangllvm added inline comments.

================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:489-491
+      // This case mostly happen in inline asm, e.g. Arr[BaseReg + IndexReg]
+      // can not intruduce additional register in inline asm in PIC model.
+      if (IsPIC && (!SymName.empty() || AttachToOperandIdx))
----------------
xiangzhangllvm wrote:
> skan wrote:
> > xiangzhangllvm wrote:
> > > skan wrote:
> > > > xiangzhangllvm wrote:
> > > > > skan wrote:
> > > > > > Why do you need to check `AttachToOperandIdx` and `SymName` at same time?
> > > > > Because in back end, (when the source code is *.ll), the GV will be present like $0 , we should use AttachToOperandIdx to make sure there is an operand.
> > > > Please add comments about this. 
> > > > 
> > > > If I understood it right, the check `!SymName.empty()` was for front end, shouldn't we add a C test for it?
> > > For the StrubPIC the SymName can also be get in the back end. Current comments is enough.  The comments should focus on what the purpose here. Not focus on explain  the SymName or other variables. (If need comments for variables, they should "attached" on the position where these variables defines)
> > But this patch is not going to support StrubPIC.  
> > Question: Is "!SymName.empty()" here covered by your tests?
> The key problem is the "IsPIC " may not works in FE, so I didn't add FE test. In FE Parser, the pic info is not yet collected.
> @jyu2 , Is there any better way to get the PIC info in front end Parser ?
Let me push this go on quickly. (For our customers are waiting for the fixing)
In fact, it is not much worth for tangling in this place for much time.

The condition here is just to give a more friendly Error report "Don't use 2 or ..." for 
(e.g.) $0[intel Expression] and Symbol[intel Expression] in PIC model.
It is also no problem to continue use the old Error report "BaseReg/IndexReg ...".

I am ok to remove the "Symbol" check if you insist on that.

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

https://reviews.llvm.org/D121785