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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei 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}"
----------------
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"`


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3528
+  ///
+  /// the operand $1 (sincos_asm) is target tranch in inline asm, but the
+  /// operand $0 (Arr) is not.
----------------
tranch -> branch


================
Comment at: llvm/include/llvm/CodeGen/TargetSubtargetInfo.h:315
+  virtual unsigned char
+  classifyGlobalFunctionReference(const GlobalValue *GV) const {
+    return 0;
----------------
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.


================
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(),
----------------
This change seems not related. Better not to modify it.


================
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);
+
----------------
This seems need re-format?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1324
+
+          // Adjust Target Flags when AdjustFlag set in inline asm.
+          if (GlobalAddressSDNode *TGA = dyn_cast<GlobalAddressSDNode>(Op)) {
----------------
Didn't see we check `AdjustFlag` here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1325
+          // Adjust Target Flags when AdjustFlag set in inline asm.
+          if (GlobalAddressSDNode *TGA = dyn_cast<GlobalAddressSDNode>(Op)) {
+            unsigned NewFlags =
----------------
RKSimon wrote:
> auto *TGA = dyn_cast<>
Comment not addressed.


================
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))
----------------
I don't think we have `call $1,...` in all case.


================
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;
----------------
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;
```


================
Comment at: llvm/test/CodeGen/X86/inline-asm-function-call-pic.ll:31
+
+; Function Attrs: nounwind uwtable
+define void @func() local_unnamed_addr #0 {
----------------
The comments don't match with actual one, please remove it.


================
Comment at: llvm/test/CodeGen/X86/inline-asm-function-call-pic.ll:62
+
+; Function Attrs: mustprogress nofree noinline norecurse nosync nounwind willreturn uwtable
+define internal i32 @static_func() #0 {
----------------
ditto.


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

https://reviews.llvm.org/D133914