This is an archive of the discontinued LLVM Phabricator instance.

[ms] [X86] Use "P" modifier on operands to call instructions in inline X86 assembly.
ClosedPublic

Authored by epastor on Dec 18 2019, 1:47 PM.

Details

Summary

This is documented as the appropriate template modifier for call operands.
Fixes PR44272, and adds a regression test.

Also adds support for operand modifiers in Intel-style inline assembly.

Diff Detail

Event Timeline

epastor created this revision.Dec 18 2019, 1:47 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 18 2019, 1:47 PM
epastor retitled this revision from Summary: Use "P" modifier on operands to call instructions in inline X86 assembly. to [ms] [X86] Use "P" modifier on operands to call instructions in inline X86 assembly..Dec 18 2019, 1:52 PM

Unit tests: pass. 60853 tests passed, 0 failed and 726 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rnk added a comment.Dec 18 2019, 2:24 PM

Where is {0:P} actually documented? I don't see it in LangRef, but I do see it in the code.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2871

I'm trying to think of other edge cases where we'd want the same treatment. In theory, we'd want to do this for every control flow instruction that takes a PC-relative operand, jmp, jcc, jecxz, that might be all. You know, it actually seems reasonable to set up a naked function that contains an asm blob which conditionally branches to another function, so I guess we should support it. In that case, maybe this should be named something like "isBranchTarget" instead of isCallTarget.

In D71677#1790458, @rnk wrote:

Where is {0:P} actually documented? I don't see it in LangRef, but I do see it in the code.

https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers, under X86; specifically documented to be used on the operands of call instructions.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2871

That would be a valid option, but currently the "P" modifier is documented for LLVM as to-be-used on the operands of call instructions. GNU as) adds it more generally to be applied to functions - and occasionally constants, if you need a constant to avoid all syntax-specific prefixes.

We could use this for any branch target operand... but we'd need to restrict it to apply only to the specific PC-relative operand, which I think means augmenting the X86 target tables to annotate which operands of which instructions are PC-relative? Also, I'm not sure if that would break pre-existing code in enough cases to be worrying.

rnk accepted this revision.Dec 21 2019, 3:21 PM

lgtm

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2871

I see, so LLVM inherited P from GCC, and it mostly relates to sym@PLT suffixes, but it also suppresses sym(%rip) suffixes, and it is specific to calls. In that case, this all makes sense, let's not overgeneralize to all branch targets.

llvm/test/CodeGen/X86/ms-inline-asm-PR44272.ll
16

I'd suggest matching for {{call(l|q) func$}} so that you don't accidentally match callq func(%rip). Which makes me wonder, does @PLT appear here? The test uses no OS in the triple, which probably means ELF, so we probably use a PLT.

This revision is now accepted and ready to land.Dec 21 2019, 3:21 PM
epastor marked 2 inline comments as done.Dec 21 2019, 8:58 PM
epastor added inline comments.
llvm/test/CodeGen/X86/ms-inline-asm-PR44272.ll
16

Thanks for that catch - completely missed that it'd match with (%rip) on the end.

Interestingly... no, no @PLT in this test. Not sure what's up with that.

epastor updated this revision to Diff 235035.Dec 21 2019, 9:02 PM
epastor marked an inline comment as done.

Fix test; missing anchor meant the previous version would not catch a regression.

Unit tests: pass. 61087 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.