[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush marked 4 inline comments as done.
mharoush added inline comments.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1382
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
rnk wrote:
> rnk wrote:
> > Please use clang-format here and elsewhere
> not addressed?
Persistent text wrapping of my editor, fixed.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1378
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
coby wrote:
> assumption ~~> assertion
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.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1380
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
coby wrote:
> I think this whole section better suites within its own function. something
> like 'ParseInlineAsmEnumValue'
not much added value here and only a few lines can be moved (~5). Its not
really worth it.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1383
+ StringRef ErrMsg;
+ // SM should treat the value as it would an explicit integer in the
+ // expression.
coby wrote:
> rephrase
Your comment is too general, I made some clarifications.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1388
+ // In case we are called on a bracketed expression,
+ if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+// A single rewrite of the integer value is preformed for each enum
coby wrote:
> 'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of
> code when parsing inline asm)
This is just for readability and consistency with SM.onInteger(). Replaced with
assertion to ease your mind
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1826
}
-
- // Rewrite the type operator and the C or C++ type or variable in terms of an
- // immediate. E.g. TYPE foo -> $$4
- unsigned Len = End.getPointer() - TypeLoc.getPointer();
- InstInfo->AsmRewrites->emplace_back(AOK_Imm, TypeLoc, Len, CVal);
-
+ // Only when in bracketed mode, preform explicit rewrite
+ if (AddImmPrefix) {
coby wrote:
> 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.
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.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1907
unsigned Len = Tok.getLoc().getPointer() - Start.getPointer();
-if (StartTok.getString().size() == Len)
- // Just add a prefix if this wasn't a complex immediate expression.
- InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Start);
-else
- // Otherwise, rewrite the complex expression as a single immediate.
+if (StartTok.getString().size() != Len || ReplaceEnumIdentifier)
+ // Rewrite the complex expression as a single immediate.
coby wrote:
> you may just perform an AOK_Imm rewrite regardless the complexity of the
> immediate expression, and neglect 'ReplaceEnumIdentifier'
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.
Repository:
rL LLVM
https://reviews.llvm.org/D33278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush updated this revision to Diff 104391.
mharoush marked an inline comment as done.
mharoush added a comment.
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 [https://reviews.llvm.org/D33277]. Made
some minor alterations and clarifications to satisfy reviewer comments.
Repository:
rL LLVM
https://reviews.llvm.org/D33278
Files:
include/llvm/MC/MCParser/MCAsmParser.h
lib/Target/X86/AsmParser/X86AsmParser.cpp
Index: lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -713,7 +713,7 @@
std::unique_ptr ParseIntelOffsetOfOperator();
bool ParseIntelDotOperator(const MCExpr *Disp, const MCExpr *);
unsigned IdentifyIntelOperator(StringRef Name);
- unsigned ParseIntelOperator(unsigned OpKind);
+ unsigned ParseIntelOperator(unsigned OpKind, bool AddImmPrefix);
std::unique_ptr
ParseIntelSegmentOverride(unsigned SegReg, SMLoc Start, unsigned Size);
std::unique_ptr ParseRoundingModeOp(SMLoc Start, SMLoc End);
@@ -1187,7 +1187,7 @@
InlineAsmIdentifierInfo , bool AllowBetterSizeMatch) {
// If we found a decl other than a VarDecl, then assume it is a FuncDecl or
// some other label reference.
- if (isa(Disp) && Info.OpDecl && !Info.IsVarDecl) {
+ if (isa(Disp) && Info.OpDecl && !Info.isVarDecl()) {
// Insert an explicit size if the user didn't have one.
if (!Size) {
Size = getPointerWidth();
@@ -1358,7 +1358,7 @@
if (OpKind == IOK_OFFSET)
return Error(IdentLoc, "Dealing OFFSET operator as part of"
"a compound immediate expression is yet to be supported");
-int64_t Val = ParseIntelOperator(OpKind);
+int64_t Val = ParseIntelOperator(OpKind,SM.getAddImmPrefix());
if (!Val)
return true;
StringRef ErrMsg;
@@ -1368,13 +1368,40 @@
PrevTK == AsmToken::RBrac) {
return false;
} else {
-InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+InlineAsmIdentifierInfo Info;
if (ParseIntelIdentifier(Val, Identifier, Info,
- /*Unevaluated=*/false, End))
+ /*Unevaluated=*/false, End))
return true;
-SM.onIdentifierExpr(Val, Identifier);
- }
- break;
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
+ // Pass the enum identifier integer value to the SM calculator.
+ if (SM.onInteger(CE->getValue(), ErrMsg))
+return Error(IdentLoc, ErrMsg);
+ // Match the behavior of integer tokens when getAddImmPrefix flag is
+ // set.
+ if (SM.getAddImmPrefix()) {
+assert(isParsingInlineAsm() &&
+ "Expected to be parsing inline assembly.");
+// A single rewrite of the integer value is preformed for each enum
+// identifier. This is only done when we are inside a bracketed
+// expression.
+size_t Len = End.getPointer() - IdentLoc.getPointer();
+InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc, Len,
+CE->getValue());
+break;
+ }
+}
+else {
+ // Notify the SM a variable identifier was found.
+ InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+ SMInfo = Info;
+ SM.onIdentifierExpr(Val, Identifier);
+}
+ } break;
}
case AsmToken::Integer: {
StringRef ErrMsg;
@@ -1452,6 +1479,7 @@
// may have already parsed an immediate displacement before the bracketed
// expression.
IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
+
if (ParseIntelExpression(SM, End))
return nullptr;
@@ -1560,6 +1588,14 @@
assert((End.getPointer() == EndPtr || !Result) &&
"frontend claimed part of a token?");
+ // Check if the search yielded a constant integer (enum identifier).
+ if (Result && Info.isConstEnum()) {
+// By creating MCConstantExpr we let the user of Val know it is safe
+// to use as an explicit constant with value = ConstVal.
+Val = MCConstantExpr::create(Info.ConstIntValue.getSExtValue(),
+ getParser().getContext());
+return false;
+ }
// If the identifier lookup was unsuccessful, assume that we are dealing with
// a label.
if (!Result) {
@@ -1758,7 +1794,7 @@
/// variable. A variable's size is the product of its LENGTH
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
coby added inline comments.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1310
}
-
-bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc )
{
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool ) {
blank line omitted
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1378
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
assumption ~~> assertion
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1380
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
I think this whole section better suites within its own function. something
like 'ParseInlineAsmEnumValue'
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1383
+ StringRef ErrMsg;
+ // SM should treat the value as it would an explicit integer in the
+ // expression.
rephrase
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1385
+ // expression.
+ if(SM.onInteger(CE->getValue(), ErrMsg))
+return Error(IdentLoc, ErrMsg);
clang format
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1388
+ // In case we are called on a bracketed expression,
+ if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+// A single rewrite of the integer value is preformed for each enum
'isParsingInlineAsm()' is unnecessary here (you can only reach this piece of
code when parsing inline asm)
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1826
}
-
- // Rewrite the type operator and the C or C++ type or variable in terms of an
- // immediate. E.g. TYPE foo -> $$4
- unsigned Len = End.getPointer() - TypeLoc.getPointer();
- InstInfo->AsmRewrites->emplace_back(AOK_Imm, TypeLoc, Len, CVal);
-
+ // Only when in bracketed mode, preform explicit rewrite
+ if (AddImmPrefix) {
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.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1907
unsigned Len = Tok.getLoc().getPointer() - Start.getPointer();
-if (StartTok.getString().size() == Len)
- // Just add a prefix if this wasn't a complex immediate expression.
- InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Start);
-else
- // Otherwise, rewrite the complex expression as a single immediate.
+if (StartTok.getString().size() != Len || ReplaceEnumIdentifier)
+ // Rewrite the complex expression as a single immediate.
you may just perform an AOK_Imm rewrite regardless the complexity of the
immediate expression, and neglect 'ReplaceEnumIdentifier'
Repository:
rL LLVM
https://reviews.llvm.org/D33278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
rnk added inline comments.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1382
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
rnk wrote:
> Please use clang-format here and elsewhere
not addressed?
Repository:
rL LLVM
https://reviews.llvm.org/D33278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush marked 3 inline comments as done. mharoush added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush marked an inline comment as done. mharoush added inline comments. Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:722 + bool ParseIntelExpression(IntelExprStateMachine , SMLoc , +bool ); std::unique_ptr rnk wrote: > mharoush wrote: > > rnk wrote: > > > Please try to eliminate the need for this extra boolean out parameter. > > 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 . > 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. I believe this should resolve the complex if statement, However I still need the boolean value to enforce the rewrite for enum identifiers. Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush updated this revision to Diff 101688.
mharoush marked an inline comment as done.
mharoush added a comment.
Simplified the AsmRewrite condition in x86AsmParser. Some fixes to simplify the
Intel State Machine immediate value rewrite treatment divergence.
Repository:
rL LLVM
https://reviews.llvm.org/D33278
Files:
include/llvm/MC/MCParser/MCAsmParser.h
lib/Target/X86/AsmParser/X86AsmParser.cpp
Index: lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -713,12 +713,13 @@
std::unique_ptr ParseIntelOffsetOfOperator();
bool ParseIntelDotOperator(const MCExpr *Disp, const MCExpr *);
unsigned IdentifyIntelOperator(StringRef Name);
- unsigned ParseIntelOperator(unsigned OpKind);
+ unsigned ParseIntelOperator(unsigned OpKind, bool AddImmPrefix);
std::unique_ptr
ParseIntelSegmentOverride(unsigned SegReg, SMLoc Start, unsigned Size);
std::unique_ptr ParseRoundingModeOp(SMLoc Start, SMLoc End);
bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine );
- bool ParseIntelExpression(IntelExprStateMachine , SMLoc );
+ bool ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool );
std::unique_ptr
ParseIntelBracExpression(unsigned SegReg, SMLoc Start, int64_t ImmDisp,
bool isSymbol, unsigned Size);
@@ -1187,7 +1188,7 @@
InlineAsmIdentifierInfo , bool AllowBetterSizeMatch) {
// If we found a decl other than a VarDecl, then assume it is a FuncDecl or
// some other label reference.
- if (isa(Disp) && Info.OpDecl && !Info.IsVarDecl) {
+ if (isa(Disp) && Info.OpDecl && !Info.isVarDecl()) {
// Insert an explicit size if the user didn't have one.
if (!Size) {
Size = getPointerWidth();
@@ -1306,8 +1307,9 @@
return false;
return true;
}
-
-bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ) {
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool ) {
+ ReplaceEnumIdentifier = false;
MCAsmParser = getParser();
const AsmToken = Parser.getTok();
@@ -1358,7 +1360,7 @@
if (OpKind == IOK_OFFSET)
return Error(IdentLoc, "Dealing OFFSET operator as part of"
"a compound immediate expression is yet to be supported");
-int64_t Val = ParseIntelOperator(OpKind);
+int64_t Val = ParseIntelOperator(OpKind,SM.getAddImmPrefix());
if (!Val)
return true;
StringRef ErrMsg;
@@ -1368,11 +1370,39 @@
PrevTK == AsmToken::RBrac) {
return false;
} else {
-InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+InlineAsmIdentifierInfo Info;
if (ParseIntelIdentifier(Val, Identifier, Info,
/*Unevaluated=*/false, End))
return true;
-SM.onIdentifierExpr(Val, Identifier);
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
+ // SM should treat the value as it would an explicit integer in the
+ // expression.
+ if(SM.onInteger(CE->getValue(), ErrMsg))
+return Error(IdentLoc, ErrMsg);
+ // In case we are called on a bracketed expression,
+ if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+// A single rewrite of the integer value is preformed for each enum
+// identifier. This is only done when we are inside a bracketed
+// expression in order to match the behavior for the equivalent
+// integer tokens.
+size_t Len = End.getPointer() - IdentLoc.getPointer();
+InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc,Len,
+CE->getValue());
+break;
+ }
+ // Set force rewrite flag only when not bracketed expression.
+ ReplaceEnumIdentifier = true;
+} else {
+ // Notify the SM a variable identifier was found.
+ InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+ SMInfo = Info;
+ SM.onIdentifierExpr(Val, Identifier);
+}
}
break;
}
@@ -1452,7 +1482,8 @@
// may have already parsed an immediate displacement before the bracketed
// expression.
IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
- if (ParseIntelExpression(SM, End))
+ bool ReplaceEnumIdentifier;
+ if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
return nullptr;
const MCExpr *Disp = nullptr;
@@ -1560,6 +1591,14 @@
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
rnk added inline comments.
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:64
unsigned ) = 0;
+ virtual bool EvaluateLookupAsEnum(void *LookupResult,int64_t ) = 0;
};
mharoush wrote:
> rnk wrote:
> > 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.
> Done
Thanks! This is better.
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:40
void *OpDecl;
bool IsVarDecl;
unsigned Length, Size, Type;
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.
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1382
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
Please use clang-format here and elsewhere
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:722
+ bool ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool );
std::unique_ptr
mharoush wrote:
> rnk wrote:
> > Please try to eliminate the need for this extra boolean out parameter.
> 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 .
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.
Repository:
rL LLVM
https://reviews.llvm.org/D33278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush updated this revision to Diff 99778.
mharoush added a comment.
Using identifier info to pass enum information.
Repository:
rL LLVM
https://reviews.llvm.org/D33278
Files:
include/llvm/MC/MCParser/MCAsmParser.h
lib/Target/X86/AsmParser/X86AsmParser.cpp
Index: lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -718,7 +718,8 @@
ParseIntelSegmentOverride(unsigned SegReg, SMLoc Start, unsigned Size);
std::unique_ptr ParseRoundingModeOp(SMLoc Start, SMLoc End);
bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine );
- bool ParseIntelExpression(IntelExprStateMachine , SMLoc );
+ bool ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool );
std::unique_ptr
ParseIntelBracExpression(unsigned SegReg, SMLoc Start, int64_t ImmDisp,
bool isSymbol, unsigned Size);
@@ -1306,8 +1307,9 @@
return false;
return true;
}
-
-bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ) {
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool ) {
+ ReplaceEnumIdentifier = false;
MCAsmParser = getParser();
const AsmToken = Parser.getTok();
@@ -1368,11 +1370,40 @@
PrevTK == AsmToken::RBrac) {
return false;
} else {
-InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+InlineAsmIdentifierInfo Info;
+Info.clear();
if (ParseIntelIdentifier(Val, Identifier, Info,
/*Unevaluated=*/false, End))
return true;
-SM.onIdentifierExpr(Val, Identifier);
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
+ // SM should treat the value as it would an explicit integer in the
+ // expression.
+ if(SM.onInteger(CE->getValue(), ErrMsg))
+return Error(IdentLoc, ErrMsg);
+ // In case we are called on a bracketed expression,
+ if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+// A single rewrite of the integer value is preformed for each enum
+// identifier. This is only done when we are inside a bracketed
+// expression in order to match the behavior for the equivalent
+// integer tokens.
+size_t Len = End.getPointer() - IdentLoc.getPointer();
+InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc,Len,
+CE->getValue());
+break;
+ }
+ // Set force rewrite flag only when not bracketed expression.
+ ReplaceEnumIdentifier = true;
+} else {
+ // Notify the SM a variable identifier was found.
+ InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+ SMInfo = Info;
+ SM.onIdentifierExpr(Val, Identifier);
+}
}
break;
}
@@ -1452,7 +1483,8 @@
// may have already parsed an immediate displacement before the bracketed
// expression.
IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
- if (ParseIntelExpression(SM, End))
+ bool ReplaceEnumIdentifier;
+ if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
return nullptr;
const MCExpr *Disp = nullptr;
@@ -1559,7 +1591,15 @@
// failed parsing.
assert((End.getPointer() == EndPtr || !Result) &&
"frontend claimed part of a token?");
-
+
+ // Check if the search yielded a constant integer (enum identifier).
+ if (Result && Info.IsConstEnum) {
+// By creating MCConstantExpr we let the user of Val know it is safe
+// to use as an explicit constant with value = ConstVal.
+Val = MCConstantExpr::create(Info.ConstIntValue.getSExtValue(),
+ getParser().getContext());
+return false;
+ }
// If the identifier lookup was unsuccessful, assume that we are dealing with
// a label.
if (!Result) {
@@ -1852,18 +1892,21 @@
AsmToken StartTok = Tok;
IntelExprStateMachine SM(/*Imm=*/0, /*StopOnLBrac=*/true,
/*AddImmPrefix=*/false);
- if (ParseIntelExpression(SM, End))
+ // The parsed expression may contain enum identifier tokens which we must
+ // replace, ReplaceEnumIdentifier flag lets us know when to force rewrite.
+ bool ReplaceEnumIdentifier;
+ if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
return nullptr;
-
bool isSymbol = SM.getSym() && SM.getSym()->getKind() != MCExpr::Constant;
int64_t Imm = SM.getImm();
if (SM.getSym() &&
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush marked an inline comment as done. mharoush added inline comments. Comment at: include/llvm/MC/MCParser/MCAsmParser.h:64 unsigned ) = 0; + virtual bool EvaluateLookupAsEnum(void *LookupResult,int64_t ) = 0; }; rnk wrote: > 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. Done Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:722 + bool ParseIntelExpression(IntelExprStateMachine , SMLoc , +bool ); std::unique_ptr rnk wrote: > Please try to eliminate the need for this extra boolean out parameter. 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 . Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
rnk added a comment. Please consider using the monorepo to upload and submit this as a single patch. Comment at: include/llvm/MC/MCParser/MCAsmParser.h:64 unsigned ) = 0; + virtual bool EvaluateLookupAsEnum(void *LookupResult,int64_t ) = 0; }; 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. Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:722 + bool ParseIntelExpression(IntelExprStateMachine , SMLoc , +bool ); std::unique_ptr Please try to eliminate the need for this extra boolean out parameter. Repository: rL LLVM https://reviews.llvm.org/D33278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax
mharoush created this revision.
This patch enables the usage of constant Enum identifiers within Microsoft
style inline assembly statements.
part 2 out of 2.
[https://reviews.llvm.org/D33277]
Repository:
rL LLVM
https://reviews.llvm.org/D33278
Files:
include/llvm/MC/MCParser/MCAsmParser.h
lib/Target/X86/AsmParser/X86AsmParser.cpp
Index: lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -718,7 +718,8 @@
ParseIntelSegmentOverride(unsigned SegReg, SMLoc Start, unsigned Size);
std::unique_ptr ParseRoundingModeOp(SMLoc Start, SMLoc End);
bool ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine );
- bool ParseIntelExpression(IntelExprStateMachine , SMLoc );
+ bool ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool );
std::unique_ptr
ParseIntelBracExpression(unsigned SegReg, SMLoc Start, int64_t ImmDisp,
bool isSymbol, unsigned Size);
@@ -1306,8 +1307,9 @@
return false;
return true;
}
-
-bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ) {
+bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine , SMLoc ,
+bool ) {
+ ReplaceEnumIdentifier = false;
MCAsmParser = getParser();
const AsmToken = Parser.getTok();
@@ -1368,11 +1370,40 @@
PrevTK == AsmToken::RBrac) {
return false;
} else {
-InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+InlineAsmIdentifierInfo Info;
+Info.clear();
if (ParseIntelIdentifier(Val, Identifier, Info,
/*Unevaluated=*/false, End))
return true;
-SM.onIdentifierExpr(Val, Identifier);
+// Check if the parsed identifier was a constant Integer. Here we
+// assume Val is of type MCConstantExpr only when it is safe to replace
+// the identifier with its constant value.
+if (const MCConstantExpr *CE =
+dyn_cast_or_null(Val)) {
+ StringRef ErrMsg;
+ // SM should treat the value as it would an explicit integer in the
+ // expression.
+ if(SM.onInteger(CE->getValue(), ErrMsg))
+return Error(IdentLoc, ErrMsg);
+ // In case we are called on a bracketed expression,
+ if (isParsingInlineAsm() && SM.getAddImmPrefix()) {
+// A single rewrite of the integer value is preformed for each enum
+// identifier. This is only done when we are inside a bracketed
+// expression in order to match the behavior for the equivalent
+// integer tokens.
+size_t Len = End.getPointer() - IdentLoc.getPointer();
+InstInfo->AsmRewrites->emplace_back(AOK_Imm, IdentLoc,Len,
+CE->getValue());
+break;
+ }
+ // Set force rewrite flag only when not bracketed expression.
+ ReplaceEnumIdentifier = true;
+} else {
+ // Notify the SM a variable identifier was found.
+ InlineAsmIdentifierInfo = SM.getIdentifierInfo();
+ SMInfo = Info;
+ SM.onIdentifierExpr(Val, Identifier);
+}
}
break;
}
@@ -1452,7 +1483,8 @@
// may have already parsed an immediate displacement before the bracketed
// expression.
IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
- if (ParseIntelExpression(SM, End))
+ bool ReplaceEnumIdentifier;
+ if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
return nullptr;
const MCExpr *Disp = nullptr;
@@ -1559,7 +1591,15 @@
// failed parsing.
assert((End.getPointer() == EndPtr || !Result) &&
"frontend claimed part of a token?");
-
+
+ // Try to evaluate the result as a constant integer (enum identifier).
+ int64_t ConstVal = 0;
+ if (SemaCallback->EvaluateLookupAsEnum(Result, ConstVal)) {
+// By creating MCConstantExpr we let the user of Val know it is safe
+// to use as an explicit constant with value = ConstVal.
+Val = MCConstantExpr::create(ConstVal, getParser().getContext());
+return false;
+ }
// If the identifier lookup was unsuccessful, assume that we are dealing with
// a label.
if (!Result) {
@@ -1852,18 +1892,21 @@
AsmToken StartTok = Tok;
IntelExprStateMachine SM(/*Imm=*/0, /*StopOnLBrac=*/true,
/*AddImmPrefix=*/false);
- if (ParseIntelExpression(SM, End))
+ // The parsed expression may contain enum identifier tokens which we must
+ // replace, ReplaceEnumIdentifier flag lets us know when to force rewrite.
+ bool ReplaceEnumIdentifier;
+ if (ParseIntelExpression(SM, End, ReplaceEnumIdentifier))
return nullptr;
-
bool isSymbol = SM.getSym() && SM.getSym()->getKind() !=
