[PATCH] D109407: [InlineAsm] Support call function label in x86 inline asm with PIC

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added a comment.

I have a general question. Can we handle it in X86DAGToDAGISel::PreprocessISelDAG()? We can create a new "X86ISD::Wrapper" and replace the corresponding memory operand of the INLINEASM node with the "X86ISD::Wrapper" node. Here is the pseudo example.

Transform the DAG

  t0: ch = EntryToken
      t12: i64 = X86ISD::WrapperRIP TargetGlobalAddress:i64<void (...)* @bar> 0 [TF=5]
    t14: i64,ch = load<(load (s64) from got)> t0, t12, undef:i64
  t10: ch,glue = inlineasm t0, TargetExternalSymbol:i64'call qword ptr ${0:P}
        ret', MDNode:ch<0x6b0f848>, TargetConstant:i64<13>, TargetConstant:i32<196622>, t14, TargetConstant:i32<12>, Register:i32 $df, TargetConstant:i32<12>, Register:i16 $fpsw, TargetConstant:i32<12>, Register:i32 $eflags

to

  t0: ch = EntryToken
      t12: i64 = X86ISD::WrapperRIP TargetGlobalAddress:i64<void (...)* @bar> 0 [TF=5]
    t14: i64,ch = load<(load (s64) from got)> t0, t12, undef:i64
    t120: i64 = X86ISD::Wrapper TargetGlobalAddress:i64<void (...)* @bar> 0
  t10: ch,glue = inlineasm t0, TargetExternalSymbol:i64'call qword ptr ${0:P}
        ret', MDNode:ch<0x6b0f848>, TargetConstant:i64<13>, TargetConstant:i32<196622>, t120, TargetConstant:i32<12>, Register:i32 $df, TargetConstant:i32<12>, Register:i16 $fpsw, TargetConstant:i32<12>, Register:i32 $eflags



================
Comment at: llvm/docs/LangRef.rst:5047
 - ``H``: Print a memory reference with additional offset +8.
-- ``P``: Print a memory reference or operand for use as the argument of a call
+- ``P``: Print branch target operand for use as the argument of a call
   instruction. (E.g. omit ``(rip)``, even though it's PC-relative.)
----------------
Is it accurate? It seems only in PIC mode and in Intel dialect, we print the symbol operand.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1189
+    if (StringRef(IDStart, IDEnd - IDStart).getAsInteger(10, Val))
+      report_fatal_error("Bad $ operand number in inline asm string: '" +
+                         Twine(AsmStr) + "'");
----------------
The syntax should be ensured by front-end when emit IR. Right?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1222
+                           "'");
+      }
+      OpNo += InlineAsm::getNumOperandRegisters(Flags) + 1;
----------------
Drop brace.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1246
+    return nullptr;
+  MachineSDNode::mmo_iterator I = MN->memoperands_begin();
+  if (!(*I)->isLoad())
----------------
Is it possible that there are multiple memory operands?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1253
+  // "call in inline" and "got load" with global address.
+  SDNode *NodeIndex = Node->getOperand(OpNo + 2).getNode();
+  SDNode *OpIndex = Op.getOperand(2).getNode();
----------------
This code is X86 target specific. It assume the sibmem address mod of X86. I think we should refine this function in X86 target.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1254
+  SDNode *NodeIndex = Node->getOperand(OpNo + 2).getNode();
+  SDNode *OpIndex = Op.getOperand(2).getNode();
+  if (cast<RegisterSDNode>(OpIndex)->getReg() ||
----------------
Why do we assume the 2nd operand is index operand? Is it always true? 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1255
+  SDNode *OpIndex = Op.getOperand(2).getNode();
+  if (cast<RegisterSDNode>(OpIndex)->getReg() ||
+      cast<RegisterSDNode>(NodeIndex)->getReg())
----------------
Why do we assume index register is noreg?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1401
+    const TargetMachine &TM = MF->getTarget();
+    if (TM.getTargetTriple().isX86() && TM.isPositionIndependent() &&
+        TM.getTargetTriple().isOSLinux())
----------------
Maybe create a target function to determine if we want to transform operands for call instruction. The function return false by default.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1486
+            TGA = getGAFromLoad(Node, i);
+          }
 
----------------
Drop brace.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1511
+                     /*IsDebug=*/false, IsClone, IsCloned);
+        }
         break;
----------------
Drop the brace.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:611
+        if (Subtarget->isPositionIndependent() &&
+            MI->getOperand(OpNo).isGlobal())
+          PrintSymbolOperand(MI->getOperand(OpNo), O);
----------------
Do we have any test case when MI->getOperand(OpNo) is local symbol?


================
Comment at: llvm/test/CodeGen/X86/inline-asm-call.ll:5
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -relocation-model=pic -code-model=large | FileCheck %s --check-prefix=CHECK-X64-LARGE
+; RUN: llc < %s -mtriple=i386-unknown-linux-gnu -relocation-model=pic -code-model=large | FileCheck %s --check-prefix=CHECK-X86-LARGE
+
----------------
Is it necessary to mix `-code-model=large` with `i386-unknown`?

gcc -mcmodel=large -m32 t2.c -m32
cc1: error: code model ‘large’ not supported in the 32 bit mode


================
Comment at: llvm/test/CodeGen/X86/inline-asm-call.ll:47
+; CHECK-X86-NEXT:    pushl %edi
+; CHECK-X86-NEXT:    .cfi_def_cfa_offset 8
+; CHECK-X86-NEXT:    pushl %esi
----------------
Add "nounwind" to avoid CFI instructions.


================
Comment at: llvm/test/CodeGen/X86/inline-asm-call.ll:82
+; CHECK-X64-LARGE-NEXT:    addq %rax, %rsi
+; CHECK-X64-LARGE-NEXT:    movabsq $sincos at GOT, %r9
+; CHECK-X64-LARGE-NEXT:    movq (%rsi,%r9), %r8
----------------
rsi hold the GOTbase. What does `$sincos at GOT` means? Is it the sincos entry offset to GOT base?


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

https://reviews.llvm.org/D109407