[PATCH] D133914: [InlineAsm][bugfix] Correct function addressing in inline asm
Xiang Zhang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 16 22:39:15 PDT 2022
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
More information about the llvm-commits
mailing list