rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
lgtm
================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2871
+ // differently when referenced in MS-style inline assembly.
+ if (Name.startswith("call") || Name.startswith("lcall")) {
+ for (size_t i = 1; i < Operands.size(); ++i) {
----------------
epastor wrote:
> rnk wrote:
> > 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.
> 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. [[
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#x86Operandmodifiers |
> 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.
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.
================
Comment at: llvm/test/CodeGen/X86/ms-inline-asm-PR44272.ll:15
+; CHECK: {{## InlineAsm Start|#APP}}
+; CHECK: call{{l|q}} func
+; CHECK: {{## InlineAsm End|#NO_APP}}
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71677/new/
https://reviews.llvm.org/D71677
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
