[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/TargetSubtargetInfo.h:315
+  virtual unsigned char
+  classifyGlobalFunctionReference(const GlobalValue *GV) const {
+    return 0;
----------------
xiangzhangllvm wrote:
> 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
Got it, thanks!


================
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);
+
----------------
xiangzhangllvm wrote:
> pengfei wrote:
> > This seems need re-format?
> Clang format ?
Yes.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8812
   for (SDISelAsmOperandInfo &OpInfo : ConstraintOperands) {
+    if (OpInfo.hasArg() || OpInfo.Type == InlineAsm::isOutput)
+      ArgNo++;
----------------
I see the GlobalISel handles argument and result separately https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp#L289-L290. No sure if there's problem to handle together here, e.g., a constrain is both argument and output https://godbolt.org/z/98sne5h4x or other corner cases?


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

https://reviews.llvm.org/D133914