Page MenuHomePhabricator

[X86][MS-InlineAsm] Extended support for variables / identifiers on memory / immediate expressions
ClosedPublic

Authored by coby on Sep 1 2017, 9:28 PM.

Details

Summary

Allow the proper recognition of Enum values and global variables inside ms inline-asm memory / immediate expressions, as they require some additional overhead and treated incorrect if doesn't early recognized.
supersedes D33278, D35774

Diff Detail

Repository
rL LLVM

Event Timeline

coby created this revision.Sep 1 2017, 9:28 PM
coby updated this revision to Diff 113658.Sep 2 2017, 2:28 PM
RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
include/llvm/MC/MCParser/MCAsmParser.h
43 ↗(On Diff #113658)

Preferred enum style is

enum IdKind {
   IK_Invalid,      // Initial state. Unexpected after a successful parsing.
   IK_Label,    // Function/Label reference.
   IK_EnumVal,  // Value of enumration type.
   IK_Var       // Variable.
 };
lib/Target/X86/AsmParser/X86AsmParser.cpp
589 ↗(On Diff #113658)
if (IDInfo.isKind(InlineAsmIdentifierInfo::EnumValKind))
  return onInteger(IDInfo.Enum.EnumVal, ErrMsg);
595 ↗(On Diff #113658)
// Treat a symbolic constant like an integer
if (auto *CE = dyn_cast<MCConstantExpr>(SymRef))
  return onInteger(CE->getValue(), ErrMsg);
1275 ↗(On Diff #113658)

This looks like its a NFC commit that can be done separately

coby added inline comments.Sep 5 2017, 4:28 AM
include/llvm/MC/MCParser/MCAsmParser.h
43 ↗(On Diff #113658)

accepted

lib/Target/X86/AsmParser/X86AsmParser.cpp
1275 ↗(On Diff #113658)

can you be a bit more specific regarding the exact part which can be commited as a NFC?
The only one i'm trivially spotting is the encapsulation of the constant symbolic reference inside of 'onIdentifierExpr', when parsing assembly (non inline-asm) content, rather than prior calling it - was that the issue you've meant?

RKSimon added inline comments.Sep 5 2017, 5:19 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1275 ↗(On Diff #113658)

I meant const InlineAsmIdentifierInfo &Info could be committed right now - it's only ever used as a const already.

coby updated this revision to Diff 114521.Sep 10 2017, 6:33 AM
coby edited the summary of this revision. (Show Details)

Addressed Simon's comments

rnk accepted this revision.Sep 11 2017, 1:32 PM

Looks good

This revision is now accepted and ready to land.Sep 11 2017, 1:32 PM
RKSimon accepted this revision.Sep 12 2017, 4:14 AM

LGTM as well, with a couple of minors.

lib/Target/X86/AsmParser/X86AsmParser.cpp
589 ↗(On Diff #113658)

This tidyup would still be nice.

595 ↗(On Diff #113658)

This tidyup would still be nice.

coby added inline comments.Sep 12 2017, 4:24 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
589 ↗(On Diff #113658)

sorry, don't know why've i missed this one out
will be added to the commit
thanks

595 ↗(On Diff #113658)

same

coby added a comment.Sep 12 2017, 4:29 AM

also, i'll appreciate if you may review/accept the (essential) frontend changes - D37413

This revision was automatically updated to reflect the committed changes.