[PATCH] D26586: [X86][AVX512][InlineASM][MS][llvm] (I|G)CC Memory adjustments compatibility

[Marina Yatsina via llvm-commits]

myatsina accepted this revision.
myatsina added a comment.
This revision is now accepted and ready to land.

LGTM (in condition you'll fix the typos and return to the break as Elena mentioned).

Also,
please clarify the summary of this review.



================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1209
+    if (IsBracketedMemOperand)
+      // handle cases where size qualifier is absent, upon an indirect symbol reference - i.e. "[var]"
+      // set Size to zero to allow matching mechansim to try and find a better
----------------
myatsina wrote:
> Please clang-format your patch.
> handle --> Handle
Still relevant:
handle --> Handle


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:771
+  /// Obtain an appropriate size qualifier, when facing a missing
+  /// one, upon AVX512 vector/broadcast memory operand
+  unsigned AdjustAVX512Mem(unsigned Size, X86Operand* UnsizedMemOpNext);
----------------
Rephrase (don't forget to clang-format):

Obtain an appropriate size qualifier, when the AVX512 vector/broadcast memory operand is missing a qualifier.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2885
   for (const auto &Op : Operands) {
     X86Operand *X86Op = static_cast<X86Operand *>(Op.get());
+    if (UnsizedMemOp && !UnsizedMemOpNext)
----------------
coby wrote:
> delena wrote:
> > IA allows only one memory operand per instruction.
> > if (UnsizedMemOp) {
> >       UnsizedMemOpNext = X86Op;
> >       break;
> > }
> > UnsizedMemOp = X86Op;
> > 
> Previous revision addressed this issue exactly, and was rejected. I found it redundant (and wrong) to continue looking past the first interception of an unsized mem op.
> Having said that - both approaches are congregated to the same behavior.
> If Marina won't furthermore object - I'm keen to revert to previous approach.
If we're returning the break then please add a comment that we handle here only cases that have one undefined memory operand, and if an instruction has more than one memory operand we will fail later on when we'll try to match an instruction.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2955
+        // Adjust AVX512 vector/broadcast memory operand,
+        // when facing te absence of a size qualifier.
+        // Match GCC behavior on respective cases.
----------------
te --> the


Repository:
  rL LLVM

https://reviews.llvm.org/D26586