[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

Wed Nov 4 17:15:49 PST 2020

LiuChen3 added inline comments.

================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2851
+    // Parse MASM style pseudo prefixes.
+    // FIXME: This prefix should only be used for MASM, not for intel-syntax.
+    if (isParsingIntelSyntax()) {
----------------
craig.topper wrote:
> epastor wrote:
> > LiuChen3 wrote:
> > > I tried to limit to MASM. But I found that the  'isParsingMSInlineAsm()' is not accurate.  And then I tried to transmit 'ParsingMSInlineAsm' information correctly in AsmPrinterInlineAsm.cpp (according to the '-fasm-blocks' option). But I was surprised to find that isParsingMSInlineAsm() is actually used as the argument of 'MatchingInlineAsm' in 'MatchAndEmitInstruction()'. This makes me confused. Should that 'MatchingInlineAsm' be 'MatchingMSInlineAsm' ?Is this MatchingInlineAsm only used by llvm-ml.
> > > It difficult to limit this to MASM at the moment. 
> > llvm-ml attempts not to touch **anything** involving inline assembly so far. The signal that MasmParser.cpp is involved is `Parser.isParsingMasm()`. However... while I can't answer the majority of this without more research, I suspect you're correct that `MatchingInlineAsm` is misnamed. We need to check this, and if so, we should rename it to avoid confusion.
> MS inline assembly is parsed twice. Once by  clang to find names of C/C++ variables. And again in the backend. GNU inline assembly is only parsed in the backend since variable names are bound explicitly and not referenced in the assembly text.
> 
> IsParsingInlineAsm is set during the clang parsing.
Thanks. That's make sense. 
So 'MatchingInlineAsm' in MatchAndEmitInstruction() more like 'MatchingMSInlineAsm' and can only be set when parsing the instructions first time. And the second time parser can not set 'setParsingMSInlineAsm(true)'. That's make difficult to limit the scope to MASM for now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441