[PATCH] D124413: [llvm-ml] Improve indirect call parsing

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

The logic seems pretty tricky to me, but I defer to Eric for that.
And I'm slowly coming to terms that brackets doesn't seem to mean anything to masm, it's really just about the operand types.

I'm basically happy, but maybe we can make the test file clearer by defining some macro with the /D option?



================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:4718
+                                   MatchingInlineAsm, isParsingIntelSyntax());
+    Match.push_back(MI);
     // If this returned as a missing feature failure, remember that.
----------------
This change looks like a no-op?


================
Comment at: llvm/lib/Target/X86/AsmParser/X86Operand.h:76
+
+    bool MaybeDirectBranchDest;
   };
----------------
A comment would be good.


================
Comment at: llvm/test/tools/llvm-ml/indirect_branch.asm:186
+t16:
+je [t16];
+; CHECK-LABEL: t16:
----------------
Do we give a decent error for "je fn_ref"?


================
Comment at: llvm/test/tools/llvm-ml/unconditional_branch_x86.asm:1
+; RUN: llvm-ml -m32 -filetype=s %s /Fo - | FileCheck %s
+
----------------
ayzhao wrote:
> hans wrote:
> > Since they're mostly the same, maybe instead of having separate test files for 32- and 64-bit, maybe it would be simpler to combine them into one. file with two "RUN" lines. 
> > You could use a macro to define e.g. "WORD" to "qword" or "dword", and use "CHECK32" and "CHECK64" for when the expectations are different.
> Done.
> 
> At this time, type aliases are defined with the `TYPEDEF` keyword, which is not yet implemented, so I ended up splitting it into multiple blocks.
Sorry, I was thinking of "macro" as in those that can be defined on the command-line, e.g. "ml /DWORD=dword". I didn't check if llvm-ml supports the /D flag, but in case it does that might be easier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124413