Page MenuHomePhabricator

[LLVM][x86][Inline Asm] - Enum support for MS syntax
ClosedPublic

Authored by mharoush on May 17 2017, 7:27 AM.

Details

Summary

This patch enables the usage of constant Enum identifiers within Microsoft style inline assembly statements.

part 2 out of 2.
[D33277]

Diff Detail

Event Timeline

rnk edited edge metadata.May 17 2017, 10:33 AM

Please consider using the monorepo to upload and submit this as a single patch.

include/llvm/MC/MCParser/MCAsmParser.h
77

It would be cleaner if InlineAsmIdentifierInfo simply contained an APInt indicating that the identifier in question was an enum, or other integral constant expression. Less callbacks to implement.

lib/Target/X86/AsmParser/X86AsmParser.cpp
722

Please try to eliminate the need for this extra boolean out parameter.

mharoush updated this revision to Diff 99778.May 22 2017, 9:15 AM

Using identifier info to pass enum information.

mharoush marked an inline comment as done.May 22 2017, 9:15 AM
mharoush added inline comments.
include/llvm/MC/MCParser/MCAsmParser.h
77

Done

lib/Target/X86/AsmParser/X86AsmParser.cpp
722

This flag is used in case we try to compile something like mov edx, A+6 ( when A is defined as ConstantEnum ) the original condition (Line:1905) will skip the rewrite since we have an identifier as the first Token, however if we try to compile 6+A it will be rewritten as expected.

I tried to solve this in different ways, I got either the exact opposite (A+6 was replaced and 6+A wasn't) or a collision of rewrites when trying to preform other forms of replacements and leaving this section intact.

I can perhaps change the way we handle general expressions to the same way we handle them under parseIntelBracExpression, meaning use the first iteration of X86AsmParser to reformat the string (write imm prefixes and replace identifiers when needed) then refactor the string to its canonical form on the second pass.

In short this was the simplest solution that worked without regression, maybe you have a better idea?

If the issue is the method signature pollution I can move this flag into the SM class as a member to indicate a rewrite is needed, however since this flag and method are limited to this context alone, I am not sure if the overhead is wanted in such case .

rnk added inline comments.May 22 2017, 9:51 AM
include/llvm/MC/MCParser/MCAsmParser.h
47–48

Please group the booleans to reduce padding. Ideally, make this an enum something like:

enum IdKind : uint8_t {
  Variable,
  Function,
  ConstInt,
};
IdKind Kind;

This is smaller and prevents nonsense states.

77

Thanks! This is better.

lib/Target/X86/AsmParser/X86AsmParser.cpp
722

I suspect there is a way to simplify the code so that this corner case no longer exists. I don't really have time to dig through the test cases to come up with it, but please make an effort.

1379

Please use clang-format here and elsewhere

mharoush updated this revision to Diff 101688.Jun 7 2017, 4:11 AM
mharoush marked an inline comment as done.

Simplified the AsmRewrite condition in x86AsmParser. Some fixes to simplify the Intel State Machine immediate value rewrite treatment divergence.

mharoush marked an inline comment as done.Jun 7 2017, 4:15 AM
mharoush added inline comments.
lib/Target/X86/AsmParser/X86AsmParser.cpp
722

I believe this should resolve the complex if statement, However I still need the boolean value to enforce the rewrite for enum identifiers.

mharoush marked 3 inline comments as done.Jun 12 2017, 4:23 AM

ping

rnk added inline comments.Jun 26 2017, 6:19 PM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1379

not addressed?

coby added inline comments.Jun 27 2017, 2:32 PM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1310

blank line omitted

1376

assumption ~~> assertion

1378

I think this whole section better suites within its own function. something like 'ParseInlineAsmEnumValue'

1381

rephrase

1383

clang format

1386

'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of code when parsing inline asm)

1823–1833

Not keen to the use of SM.getAddImmPrefix() as a mean of distinguish whether we are parsing a bracketed expression. I know it is currently turned on when parsing it, but it isn't asserted/guaranteed.
Regardless - I'm pretty sure we can manage without this rewrite, or at the very least - should, now that TYPE/LENGTH/SIZE are part of the State Machine.

1905–1906

you may just perform an AOK_Imm rewrite regardless the complexity of the immediate expression, and neglect 'ReplaceEnumIdentifier'

mharoush updated this revision to Diff 104391.Jun 28 2017, 5:59 AM
mharoush marked an inline comment as done.

simplified the rewrite condition of complex expressions and eliminated the need to use the ReplaceEnumIdentifier flag. This requires making small adjustments to some of the older test cases seen in [D33277]. Made some minor alterations and clarifications to satisfy reviewer comments.

mharoush marked 4 inline comments as done.Jun 28 2017, 6:00 AM
mharoush added inline comments.
lib/Target/X86/AsmParser/X86AsmParser.cpp
1376

I can assert the Identifier Info is indeed an enum. However, this is a general handle for any kind of value that may by passed back from the identifier parsing. There is no point in this assertion here since the value of type MCConstantExper is always safe. We simply make the decision at the parse identifier method.

1378

not much added value here and only a few lines can be moved (~5). Its not really worth it.

1379

Persistent text wrapping of my editor, fixed.

1381

Your comment is too general, I made some clarifications.

1386

This is just for readability and consistency with SM.onInteger(). Replaced with assertion to ease your mind

1823–1833

I meant this flag is only set when parsing bracketed expressions. However, I will rephrase the comment since we only care about the AddImmPrefix flag, which reflects the SM matching flag. The objective here is to match the behavior required on integer constants when the SM flag is set.

1905–1906

Thanks, this seems fine after the condition refinement. Just had to change some of the older inline asm tests which expects the IR statement to contain a binary/octal/hex base immediate value which this rewrite will replace with the decimal value.

mharoush marked 3 inline comments as done.Jun 28 2017, 6:01 AM
mcrosier resigned from this revision.Jul 6 2017, 10:15 AM
mharoush updated this revision to Diff 106801.Jul 16 2017, 7:02 AM
mharoush removed a reviewer: mcrosier.
mharoush removed a subscriber: cfe-commits.

Cosmetics + ping

rnk accepted this revision.Jul 20 2017, 9:48 AM

lgtm

Thanks @coby for finding the simplification!

lib/Target/X86/AsmParser/X86AsmParser.cpp
1397–1398

clang-format this

1905–1906

Nice. :)

This revision is now accepted and ready to land.Jul 20 2017, 9:48 AM

FYI, rL308966 has CRLF-ended paragraph in ParseIntelExpression().

lib/Target/X86/AsmParser/X86AsmParser.cpp
1371

Is it initialized properly?

1592

Info might be uninitialized here.

coby added inline comments.Jul 25 2017, 8:57 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1371

yes, but way down along the calling sequence (LookupInlineAsmIdentifier, lib/Sema/SemaStmtAsm,cpp)

1592

see previous note

coby closed this revision.Sep 3 2017, 5:10 AM
coby edited edge metadata.

superseded by D37412