[PATCH] D33278: [LLVM][x86][Inline Asm] - Enum support for MS syntax

[Reid Kleckner via Phabricator via llvm-commits]

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