[PATCH] D133914: [InlineAsm][bugfix] Correct function addressing in inline asm

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3524
+  ///
+  /// call void asm "lea r8, $0\0A\09call qword ptr ${1:P}\0A\09ret",
+  ///               "*m,*m,~{r8},~{dirflag},~{fpsr},~{flags}"
----------------
pengfei wrote:
> How about the target branch is used by other instructions? E.g.
> `call void asm "lea r8, $0\0A\09call qword ptr ${0:P}\0A\09ret"`
We shouldn't generate the different kind uses (target branch use and data use) with same operand index. Even they are some value. 
For example the intel inline asm will generate different index for different uses (even same value).
For user who manually write the index should appoint different index (with different constraints) for different kind uses.


================
Comment at: llvm/include/llvm/CodeGen/TargetSubtargetInfo.h:315
+  virtual unsigned char
+  classifyGlobalFunctionReference(const GlobalValue *GV) const {
+    return 0;
----------------
pengfei wrote:
> xiangzhangllvm wrote:
> > xiangzhangllvm wrote:
> > > LuoYuanke wrote:
> > > > RKSimon wrote:
> > > > > Add a description comment
> > > > It seems each target has it's own defintion for the return value. Is it interpreted by target when it is used?
> > > Yes, Each target has it's own definition of this function to handle call instruction. This is a virtual function on the base class TargetSubtargetInfo. The target will call their self's classifyGlobalFunctionReference for handle function.
> > Sure
> Where's X86 implementation? I didn't find it.
Exist code, Not in the patch:
Target/X86/X86Subtarget.cpp: X86Subtarget::classifyGlobalFunctionReference


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:411
     MIB.addRegMask(RM->getRegMask());
-  } else if (GlobalAddressSDNode *TGA = dyn_cast<GlobalAddressSDNode>(Op)) {
+  } else if (auto *TGA = dyn_cast<GlobalAddressSDNode>(Op)) {
     MIB.addGlobalAddress(TGA->getGlobal(), TGA->getOffset(),
----------------
pengfei wrote:
> This change seems not related. Better not to modify it.
Sorry, this refine should be at line 1325


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1321-1322
+          SDValue Op = Node->getOperand(i);
+          AddOperand(MIB, Op, 0, nullptr, VRBaseMap,
+                     /*IsDebug=*/false, IsClone, IsCloned);
+
----------------
pengfei wrote:
> This seems need re-format?
Clang format ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:32291
+    // sub-string, e.g. "$12" contain "$1"
+    if (AsmStr.contains(OpNoStr1 + ",") || AsmStr.endswith(OpNoStr1) ||
+        AsmStr.contains(OpNoStr2))
----------------
pengfei wrote:
> I don't think we have `call $1,...` in all case.
here handle 3 case: $1, ${1: and $1 in the end of string.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:32301-32306
+  StringRef InstrStr = getInstrStrFromOpNo(AsmStrs, OpNo);
+
+  // Do not use "startswith" here, because label may stay ahead.
+  // For example: ".L__MSASMLABEL_.${:uid}__l:call dword ptr ${0:P}"
+  if (InstrStr.contains("call"))
+    return true;
----------------
pengfei wrote:
> The code seems fragile. How about using regex:
> ```
> std::string Call = formatv("call.*\\${?{0}", OpNo);
> for (auto &AsmStr : AsmStrs)
>   if (Regex(Call).match(AsmStr))
>     return true;
> return false;
> ```
good idea! Let me have a try, thanks a lot!


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

https://reviews.llvm.org/D133914