[PATCH] D35774: [x86][inline-asm]Extend support for memory reference expression

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1281
+  bool Found = false;
+  for (AsmRewrite &AR : AsmRewrites)
+    if (AR.Kind == AOK_Imm || AR.Kind == AOK_ImmPrefix) {
----------------
Please keep the curly braces on the for loop. I know elsewhere they are omitted, but this is just too clever for me.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1283
+    if (AR.Kind == AOK_Imm || AR.Kind == AOK_ImmPrefix) {
+      if ((AR.Loc.getPointer() < BracLoc.getPointer()) &&
+          (Start.getPointer() <= AR.Loc.getPointer())) {
----------------
If we have to keep doing comparing source locations, this code would greatly benefit from some helper functions that do things like the following: isLocBetween, isLocAfter, isLocBefore, so you don't have to deal in pointers all the time.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1308-1309
+
   const char *SymLocPtr = SymName.data();
-  // Skip everything before the symbol.
-  if (unsigned Len = SymLocPtr - StartInBrac.getPointer()) {
-    assert(Len > 0 && "Expected a non-negative length.");
-    AsmRewrites.emplace_back(AOK_Skip, StartInBrac, Len);
-  }
-  // Skip everything after the symbol.
-  if (unsigned Len = End.getPointer() - (SymLocPtr + SymName.size())) {
-    SMLoc Loc = SMLoc::getFromPointer(SymLocPtr + SymName.size());
-    assert(Len > 0 && "Expected a non-negative length.");
+  int Len = SymLocPtr - BracLoc.getPointer();
+
----------------
This is too clever. IMO we should take SymName, make an SMLoc out of it, and use the comparison helpers to figure out if the string is before or after '[', and do the appropriate rewrite.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1525
+                                   SymSM->getIdentifierInfo().IsGlobalLV))
+      return ErrorOperand(BracLoc, "Can't use a local variable with both "
+                         "base and index registers");
----------------
User-facing diagnostics are supposed to be sentence fragments that start with lower case. We also try to avoid contractions in user-facing diagnostics, so this should be "cannot" instead of "Can't".


Repository:
  rL LLVM

https://reviews.llvm.org/D35774