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

[Xiang Zhang via Phabricator via llvm-commits]

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))
----------------
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.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:840-853
+        // The InineAsm can also be parsed from emitInlineAsm (not from FE)
+        // which may generate "Sym - BaseSym" for pic-base-offset addressing
+        // model (e.g. i386-darwin).
+        // Here we remove "- BaseSym" in inlineasm if it exists.
+        // TODO(RefineMe): Here is an architecture defect for both FE parser
+        // and BE emitInlineAsm share the same way to parse the inline asm.
+        // So the 2 symbol syntax checking may not be always right.
----------------
skan wrote:
> Split this workaround to another patch and add a negative test for StubPIC . We'd better to remind the user that something goes wrong here.
Make sense


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

https://reviews.llvm.org/D121785