rnk added inline comments.
================
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:64
unsigned &Offset) = 0;
+ virtual bool EvaluateLookupAsEnum(void *LookupResult,int64_t &Result) = 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<const MCConstantExpr>(Val)) {
+ StringRef ErrMsg;
----------------
Please use clang-format here and elsewhere
================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:722
+ bool ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End,
+ bool &ReplaceEnumIdentifier);
std::unique_ptr<X86Operand>
----------------
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
