[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks! I made some comments. This code is convoluted. I don't think I can reason through all the edge cases, but this seems like an improvement. I'd suggest adding the recommended tests and any other corner case inputs you can think of, and after another round of review we can land it.



================
Comment at: clang/test/CodeGen/ms-inline-asm-64.c:18
 // CHECK: call void asm sideeffect inteldialect
-// CHECK-SAME: mov [eax], $0
+// CHECK-SAME: mov qword ptr [eax], $0
 // CHECK-SAME: "r,~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}})
----------------
I guess this has the same semantics as it did before.


================
Comment at: llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h:72
+      : NeedBracs(false), Imm(0), BaseReg(StringRef()), IndexReg(StringRef()),
+        OffsetName(StringRef()), Scale(1) {}
+  // [BaseReg + IndexReg * ScaleExpression + OFFSET name + ImmediateExpression]
----------------
These explicit empty StringRef initializations seem unnecessary, I'd suggest removing them.


================
Comment at: llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h:97
   SMLoc Loc;
   unsigned Len;
   int64_t Val;
----------------
nit, pack the bool after the unsigned for a smaller struct. Hey, we make a vector of these. :)


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5935-5940
+        auto rewrite_it = it;
+        while (++rewrite_it != AsmStrRewrites.end() &&
+               rewrite_it->Loc.getPointer() <= AR.IntelExp.OffsetName.data()) {
+          if (OffsetName.data() == rewrite_it->Loc.getPointer() &&
+              OffsetName.size() == rewrite_it->Len &&
+              rewrite_it->Kind == AOK_Input) {
----------------
Maybe this would be simpler with
  auto rewrite_it = find_if(it, AsmStrRewrites.end(), [&](const AsmRewrite &OffsetAR) {
    // ... conditions below
  });


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1481
+    InlineAsmIdentifierInfo Info;
+    if ((ParseError = ParseIntelOffsetOperator(Val, ID, Info, End)))
+      return true;
----------------
While this doesn't trigger warnings, it seems like it would be simpler to do the assignment as its own statement and then `if (ParseErrror) return true;`

I see that the local code style uses this pattern already, but I'm not sure that's a good reason to promote it and keep using it.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1484
+    StringRef ErrMsg;
+    if ((ParseError = SM.onOffset(Val, OffsetLoc, ID, Info,
+                                  isParsingInlineAsm(), ErrMsg)))
----------------
ditto


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1821
+        getParser().parsePrimaryExpr(Val, End))
+      return Error(Start, "unexpected token!");
+  } else if (ParseIntelInlineAsmIdentifier(Val, ID, Info, false, End, true)) {
----------------
Please test this corner case, I imagine it looks like:
  mov eax, offset 3


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1825
+  } else if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal)) {
+    return Error(Start, "offset operator cannot yet handle constants");
+  }
----------------
Please add a test for this corner case, I'm curious to see if the error reporting really works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71436/new/

https://reviews.llvm.org/D71436