Page MenuHomePhabricator

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

Authored by coby on Sep 1 2017, 9:47 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 D33277, D35775

Diff Detail

Repository
rL LLVM

Event Timeline

coby created this revision.Sep 1 2017, 9:47 PM
coby updated this revision to Diff 113659.Sep 2 2017, 2:30 PM
coby added a child revision: D37422: test.Sep 3 2017, 5:44 AM
RKSimon added a subscriber: RKSimon.

Tests?

lib/Sema/SemaStmtAsm.cpp
71 ↗(On Diff #113659)

This all looks like a clang-format NFC change - just commit it?

617 ↗(On Diff #113659)

(style) Split these instead of an if-elseif chain

coby updated this revision to Diff 114520.Sep 10 2017, 6:32 AM

addressed Simon's comments

coby edited the summary of this revision. (Show Details)Sep 10 2017, 6:32 AM
coby updated this revision to Diff 114536.Sep 10 2017, 11:59 PM
coby added reviewers: myatsina, m_zuckerman.
rnk added inline comments.Sep 12 2017, 11:03 AM
lib/Sema/SemaStmtAsm.cpp
620 ↗(On Diff #114536)

This conditional expression is a bit too cute, I'd rather use a regular if.

617 ↗(On Diff #113659)

LLVM suggests early returns: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

I don't particularly care here, up to you. I personally like to abuse the ability of C++ to return void to write things like this and skip braces, but maybe *I'm* too clever for my own good:

if (T->isFunctionType() || T->isDependentType())
  return Info.setLabel(Res);
if (Res->isRValue()) {
  if (isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context))
    return Info.setEnum(Eval.Val.getInt().getSExtValue());
  return Info.setLabel(Res);
}
coby added inline comments.Sep 12 2017, 11:56 PM
lib/Sema/SemaStmtAsm.cpp
620 ↗(On Diff #114536)

ah.. can't blame me for trying :)

617 ↗(On Diff #113659)

brilliant. was holding myself from emitting such returns, as i had the impression such kind of things are considered taboo. hate redundant bracs. will happily address that.
Anything else?

coby updated this revision to Diff 115564.Sep 17 2017, 12:53 AM

addressed @rnk 's suggestions:
cuteness out
c++ mischief in

rnk accepted this revision.Sep 19 2017, 11:58 AM

lgtm, thanks!

This revision is now accepted and ready to land.Sep 19 2017, 11:58 AM
This revision was automatically updated to reflect the committed changes.