This is an archive of the discontinued LLVM Phabricator instance.

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

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



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


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.
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.
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
43 ↗(On Diff #113658)


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
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.

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
589 ↗(On Diff #113658)

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

595 ↗(On Diff #113658)


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.