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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: clang/test/CodeGen/ms-inline-asm-functions.c:25
   __asm call kimport;
-  // CHECK:     calll   *({{.*}})
+  // CHECK:     calll   __imp__kimport
 
----------------
pengfei wrote:
> Why is it affected? Is missing a `*` before the label?
The dllimport mark it is a inter linkage, so we can directly call this function.


================
Comment at: clang/test/CodeGen/ms-inline-asm-functions.c:35
   // CHECK-LABEL: _bar:
   __asm jmp k;
   // CHECK:     jmp     _k
----------------
pengfei wrote:
> > Due to currently inline asm didn't support jmp the outsider lable, this patch
> > mainly focus on fix the function call addressing bugs in inline asm.
> 
> This seems a counter example.
This will be bug in pic model.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3497
 
+  virtual bool
+  isInlineAsmTargetBranch(const SmallVectorImpl<StringRef> &AsmStrs,
----------------
RKSimon wrote:
> RKSimon wrote:
> > Add a description comment
> Add a description comment
Sure, I'll add it, thanks.


================
Comment at: llvm/include/llvm/IR/InlineAsm.h:337
+    assert((isMemKind(InputFlag) || isFuncKind(InputFlag)) &&
+           "InputFlag is not a memory constraint!");
     assert(Constraint <= 0x7fff && "Too large a memory constraint ID");
----------------
RKSimon wrote:
> update assertion message?
Ok, let me update it. Before this patch the function is belong to Mem kind.
Currently we split them. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8761
+  SmallVector<StringRef> AsmStrs;
+  IA->collectAsmStrs(AsmStrs);
+
----------------
pengfei wrote:
> No need to define function that only called once.
The other target must have some bugs, so I create this function for further use.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8806
+        const GlobalValue *GV = GA->getGlobal();
+        IsFunc = GV && dyn_cast<Function>(GV);
+      }
----------------
RKSimon wrote:
> isa_and_nonnull<Function> ?
OK, good style!


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8811
+    // Some targets may see function as direct address not indirect mem.
+    if (IsFunc && TLI.isInlineAsmTargetBranch(AsmStrs, ArgNo)) {
+      OpInfo.isIndirect = false;
----------------
pengfei wrote:
> Can we just pass the string and handle it in the function?
Yes, fold upper code into a function call is better!


================
Comment at: llvm/test/CodeGen/X86/inline-asm-p-constraint.ll:14
+; CHECK-NEXT: lea -8(%rsp), %rax
 ; CHECK-NEXT: #NO_APP
   ret ptr %1
----------------
jonpa wrote:
> xiangzhangllvm wrote:
> > @jonpa I think here you may mis-understand lea, so I updated here.
> > And I think the mov (%rdi) should be mov %rdi in constrain of "p" you add before.
> > 
> > At first I updated the 'p' action to "direct action" (without load: mov %rdi), but it affect systemZ tests which I am not familiar with. So I want to discuss with you about the action of "p" here.
> > 
> > "p" should be address which equal with "direct mem (without load)".
> >  So in DAG build (pls refer the change in SelectionDAGBuilder.cpp) the OpInfo.isIndirect should always be false;
> Yes, as far as I understand "p" (address) is not "indirect" - the address is specified at the source level and not used to access memory. It's when you pass an address to a memory operand the "indirect" flag is added.
> 
Yes, but now I pass 'p' for "ptr %Ptr" in ""mov $1", it still load the context of "ptr %Ptr".
So this is a problem.
I prefer to fix it.


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

https://reviews.llvm.org/D133914