skan marked 3 inline comments as done.
skan added inline comments.
================
Comment at: clang/test/CodeGen/ms-inline-asm-static-variable.c:8
+ // CHECK: @arr = internal global [10 x i32]
+ // CHECK: call void asm sideeffect inteldialect "mov dword ptr $0[edx *
$$4],edx", "=*m,{{.*}}([10 x i32]* @arr)
+ __asm mov dword ptr arr[edx*4],edx
----------------
pengfei wrote:
> skan wrote:
> > pengfei wrote:
> > > IIRC, we have limitation on the number. Can you check if it works when we
> > > have more than 10 global variables.
> > sorry, which number?
> `$0`. I mean if we have arr1, arr2, ..., we should have `$1`, `$2`, ... but
> the max number is `$9` if I remember it correctly.
We does not touch that part of the code. If there was this limitation before,
there is now.
================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1759
// It is widely common for MS InlineAsm to use a global variable and one/two
// registers in a mmory expression, and though unaccessible via rip/eip.
if (IsGlobalLV && (BaseReg || IndexReg)) {
----------------
xiangzhangllvm wrote:
> Let me generally tell out my understand here, (If wrong PLS correct me)
> Here from the comments we can see, the old code want to keep the origin
> symbol of global variable to let linker (relocation) handle it. Here you
> describe it with a pointer (with decl), it change to form of $ID <-->
> (decl), So which need constrain it with "*m". But if the pointer can not be
> access from BaseReg(Rip) + Index(Ip) how do you descript the pointer you
> generate out ?
>
I think you may misunderstand this code.
This code handles the memory that can not be represented by Disp(RIP) b/c there
is already a BaseReg or IndexReg there.
Before this patch, the memory is represented like `arr[edx*4]` and there is no
identifer bound to it. And after this patch, we bind the memory to identifer
arr.
================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2554-2555
unsigned IndexReg = SM.getIndexReg();
+ if (IndexReg && BaseReg == X86::RIP)
+ BaseReg = 0;
unsigned Scale = SM.getScale();
----------------
xiangzhangllvm wrote:
> The change here looks too arbitrary. For global address it is ok to drop the
> base, it mainly fetch from offset. but if here not global variable?
RIP and IndexReg can never be used together according to design of X86
instruction. The BaseReg is set to RIP b/c we add the constaint "*m" in the
MS-inline assembly. So we can drop it safely.
I acknowledge it's not the best way to do it, but it's simplest. Similary, you
can see the line 2560-2562. RSP can never be a IndexReg, but we just swap the
BaseReg w/ IndexReg b/c it is not handled well in previous phases.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113096/new/
https://reviews.llvm.org/D113096
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
