This is an archive of the discontinued LLVM Phabricator instance.

The [3/3] Fix mangle problem when variable used in inline asm (Support ARR[BaseReg+IndexReg+..] in PIC model)
ClosedPublic

Authored by xiangzhangllvm on Mar 16 2022, 3:10 AM.

Details

Summary

This patch is base on D120886 and D120887

These 2 patches let Global Value inline asm operand visible in inline asm code string.
But they bring some problems in building inline asm in PIC model.
(Some problems of building inline asm in PIC model are always existed for a long time.)

There are 3 kind PIC models in X86 : GOTPIC, RIPPIC and StubPIC
This patch support building Global Value inline asm operand in GOTPIC, RIPPIC PIC model.

For StubPIC , there is always incorrect in using global value in inline asm.
For example, the mangle and address calculation is incorrect in
https://godbolt.org/z/3T4PGdeqM 
I will fix it in another patch if we have such requirement.

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Mar 16 2022, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 3:10 AM
xiangzhangllvm requested review of this revision.Mar 16 2022, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 3:10 AM
xiangzhangllvm edited the summary of this revision. (Show Details)
skan added inline comments.Mar 21 2022, 8:20 PM
llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-1.ll
1 ↗(On Diff #416849)

Please use a more specific name for the test files. "-1" is ambiguous.

skan added inline comments.Mar 21 2022, 8:46 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

Why do you need to check AttachToOperandIdx and SymName at same time?

skan added inline comments.Mar 21 2022, 8:59 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
841–842

Split this workaround to another patch and add a negative test for StubPIC . We'd better to remind the user that something goes wrong here.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

Because in back end, (when the source code is *.ll), the GV will be present like $0 , we should use AttachToOperandIdx to make sure there is an operand.

841–842

Make sense

skan added inline comments.Mar 21 2022, 10:49 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

Please add comments about this.

If I understood it right, the check !SymName.empty() was for front end, shouldn't we add a C test for it?

844

inline asm

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

For the StrubPIC the SymName can also be get in the back end. Current comments is enough. The comments should focus on what the purpose here. Not focus on explain the SymName or other variables. (If need comments for variables, they should "attached" on the position where these variables defines)

xiangzhangllvm marked 3 inline comments as done.Mar 21 2022, 11:08 PM
skan added inline comments.Mar 21 2022, 11:10 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

But this patch is not going to support StrubPIC.
Question: Is "!SymName.empty()" here covered by your tests?

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

The key problem is the "IsPIC " may not works in FE, so I didn't add FE test. In FE Parser, the pic info is not yet collected.
@jyu2 , Is there any better way to get the PIC info in front end Parser ?

xiangzhangllvm added inline comments.Mar 22 2022, 6:24 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
489–491

Let me push this go on quickly. (For our customers are waiting for the fixing)
In fact, it is not much worth for tangling in this place for much time.

The condition here is just to give a more friendly Error report "Don't use 2 or ..." for
(e.g.) $0[intel Expression] and Symbol[intel Expression] in PIC model.
It is also no problem to continue use the old Error report "BaseReg/IndexReg ...".

I am ok to remove the "Symbol" check if you insist on that.

xiangzhangllvm marked 2 inline comments as done.
skan added inline comments.Mar 22 2022, 8:42 PM
llvm/docs/LangRef.rst
5082–5084

It -> it, edditional ->additional.

Print a memory reference or operand for use as the argument of a call instruction (E.g. omit `(rip)`, even though it's PC-relative) or a memory reference with explict base reg and index reg.

xiangzhangllvm marked an inline comment as done.
skan added a comment.Mar 22 2022, 11:36 PM

Added @jyknight as reviewer to let him know the sematic of modifier P is extended in this patch. The documentation of P was added in D10816.

skan accepted this revision.Mar 22 2022, 11:44 PM

LGTM except the doc part.

llvm/docs/LangRef.rst
5082

I suggest the following doc:

Print a memory reference or operand for use as the argument of a call instruction (e.g. omit (rip), even though it's PC-relative) or a memory reference with explict base reg and index reg.

This revision is now accepted and ready to land.Mar 22 2022, 11:44 PM

Added @jyknight as reviewer to let him know the sematic of modifier P is extended in this patch. The documentation of P was added in D10816.

This patch is in at 7 years ago (2015), I don't think the author remember so details. What's more, this is x86 only modifier and it is time to update it today (with finding so many defects in inline asm)

This revision was landed with ongoing or failed builds.Mar 23 2022, 6:42 PM
This revision was automatically updated to reflect the committed changes.