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

Mon Mar 21 23:10:57 PDT 2022

skan 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:
> > > > 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?

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

https://reviews.llvm.org/D121785