[PATCH] D33277: [Clang][x86][Inline Asm] - Enum support for MS syntax

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:674
+  Expr::EvalResult EvlResult;
+  // Try to evaluate the identifier as enum constant
+  if (isa<clang::EnumType>(Res->getType()) && Res->EvaluateAsRValue(EvlResult, 
----------------
Please add a comment explaining that non-enum integer constants should not be folded. I expected that MSVC would fold constexpr integers here, but it does not, so that's worth noting.


================
Comment at: test/CodeGen/x86-ms-inline-asm-enum_feature.cpp:12
+  const int a = 0;
+  // CHECK-NOT: mov eax, [$$0]
+  __asm mov eax, [a]
----------------
mharoush wrote:
> rnk wrote:
> > Use CHECK-LABEL, CHECK, and CHECK-SAME the way that the existing ms-inline-asm.c tests do so that this test is easier to debug when it fails.
> This test case was just meant verify that other Integer constants are not folded since we get a different behavior for statements such as mov eax, [a]. 
> #---
> In this example X86AsmParser regards the address of the variable 'a' and not its value i.e. we end up with the value of 'a' in eax (loaded from the stack) and not with the value pointed by the const int value of 'a' as its address.
> ---#
> 
> I can clarify the intention in a comment or completely remove the test case since this isn't really required here.
The test case is fine and the intention is clear, I am just suggesting that you use more FileCheck features (CHECK-LABEL and CHECK-SAME) to make the test easier to debug when it fails in the future.


Repository:
  rL LLVM

https://reviews.llvm.org/D33277