[x86][inline-asm][llvm]Amend size directive deduction mechanism of unsized memory operands

[coby]

Summary

This is an extension of the work being carried by the following change:
https://reviews.llvm.org/D26586
This commit handles cases where the size qualifier of an indirect memory reference operand in Intel syntax is missing (e.g. "vaddps xmm1, xmm2, [a]").
GCC will deduce the size qualifier based on the possible matches:
"vaddps xmm1, xmm2, [a]" matches only “XMMWORD PTR” qualifier.
"vaddps xmm1, xmm2, [a]{1to4}" matches only “DWORD PTR” qualifier.
"mov rax, [a]" matches only "QWORD PTR"

Currently, size directive will be deduced based on the size of the memory operand (apart from those cases which were handled by D26586).
For example:
"vaddps xmm1, xmm2, [a]"
"char a;" will imply "BYTE PTR" qualifier
"short a;" will imply "WORD PTR" qualifier.

This commit aligns LLVM to GCC’s behavior.

This is the LLVM part of the review.
Clang part can be found here:
https://reviews.llvm.org/D32638

[rnk]

GCC doesn't support Intel inline asm, so I'm confused how we can be aligning to their behavior. Are we matching gas, gcc, or msvc here?

[coby]

@rnk

consider the following code:

char c;
void foo() {
__asm("mov rax, [c]");
}

compiled with -masm=intel, using GCC 7.1
is lowered to (intel style disassembly):
mov rax, qword ptr c_address
similarly - you can achieve the same effect by using the following directive (and the 'noprefix' option) - ".intel_syntax noprefix"

Now, as GCC merely pass the bucket to AS, you may say it is AS we are matching
Equivalently, you may say we actually match GCC, as inline-asm is (also) a high-level-source-code entity - i.e. we temper inline-asm here only - we do not affect actions being carried at an assembly level.
I'm more keen to support the second statement, but it's hardly matters.

[rnk]

I think we had a user file this bug at https://bugs.llvm.org//show_bug.cgi?id=28266

Is this change testable from LLVM?

[rnk]

rL166316 was the commit that originally added the AOK_SizeDirective logic for inline asm. @mcrosier said "This allows the backend to do proper instruction selection." I think now that we have the unsized mem operand matching loop, we don't need this rewrite at all, it's just getting in the way.

[rnk]

In D32636#746140, @rnk wrote:

rL166316 was the commit that originally added the AOK_SizeDirective logic for inline asm. @mcrosier said "This allows the backend to do proper instruction selection." I think now that we have the unsized mem operand matching loop, we don't need this rewrite at all, it's just getting in the way.

I take this back, we do need it. I had a vague memory that MSVC doesn't do any of this memory operand size disambiguation based on frontend information, but that's not true. There are a few instructions that take registers, but can have varying memory operand size, like movzx:

void t13() {
  char i = 1;
  short j = 2;
  __asm movzx eax, [i]
  __asm movzx eax, [j]
}

MSVC's object file disassembles as:

00000000: 0F B6 05 00 00 00  movzx       eax,byte ptr [_i]
          00
00000007: 0F B7 05 00 00 00  movzx       eax,word ptr [_j]
          00

We need the size from the frontend to disambiguate this.

[rnk]

Does rL302179 solve the problems you were having? It should reduce the number of places we need to do these ugly rewrites, and make inline asm parsing more consistent with standalone asm parsing.

[coby]

superseded by rL302179

lib/Target/X86/AsmParser/X86AsmParser.cpp

private:
   /// Parses AVX512 specific operand primitives: masked registers ({%k<NUM>}, {z})
   /// and memory broadcasting ({1to<NUM>}) primitives, updating Operands vector if required.
   /// return false if no parsing errors occurred, true otherwise.
   bool HandleAVX512Operand(OperandVector &Operands,
                            const MCParsedAsmOperand &Op);
 
   bool ParseZ(std::unique_ptr<X86Operand> &Z, const SMLoc &StartLoc);
 
-  /// MS-compatibility:
-  /// Obtain an appropriate size qualifier, when facing its absence,
-  /// upon AVX512 vector/broadcast memory operand
-  unsigned AdjustAVX512Mem(unsigned Size, X86Operand* UnsizedMemOpNext);
-
   bool is64BitMode() const {
     // FIXME: Can tablegen auto-generate this?
     return getSTI().getFeatureBits()[X86::Mode64Bit];
   }
   bool is32BitMode() const {
     // FIXME: Can tablegen auto-generate this?
     return getSTI().getFeatureBits()[X86::Mode32Bit];
   }
std::unique_ptr<X86Operand> X86AsmParser::CreateMemForInlineAsm(
   if (IsSymRef) {
     if (!Size) {
       Size = Info.Type * 8; // Size is in terms of bits in this context.
       if (Size)
         InstInfo->AsmRewrites->emplace_back(AOK_SizeDirective, Start,
                                             /*Len=*/0, Size);
     if (AllowBetterSizeMatch)
       // Handle cases where size qualifier is absent, upon an indirect symbol
-      // reference - e.g. "vaddps zmm1, zmm2, [var]"
+      // reference, e.g. "vaddps zmm1, zmm2, [var]"
       // set Size to zero to allow matching mechansim to try and find a better
       // size qualifier than our initial guess, based on available variants of
       // the given instruction
       Size = 0;
     }
   }
 
   // When parsing inline assembly we set the base register to a non-zero value
bool X86AsmParser::MatchAndEmitATTInstruction(SMLoc IDLoc, unsigned &Opcode,
   }
 
   // If all of these were an outright failure, report it in a useless way.
   Error(IDLoc, "unknown use of instruction mnemonic without a size suffix",
         EmptyRange, MatchingInlineAsm);
   return true;
 }
 
-unsigned X86AsmParser::AdjustAVX512Mem(unsigned Size,
-    X86Operand* UnsizedMemOpNext) {
-  // Check for the existence of an AVX512 platform
-  if (!getSTI().getFeatureBits()[X86::FeatureAVX512])
-    return 0;
-  // Allow adjusting upon a (x|y|z)mm
-  if (Size == 512 || Size == 256 || Size == 128)
-    return Size;
-  // This is an allegadly broadcasting mem op adjustment,
-  // allow some more inquiring to validate it
-  if (Size == 64 || Size == 32)
-    return UnsizedMemOpNext && UnsizedMemOpNext->isToken() &&
-      UnsizedMemOpNext->getToken().substr(0, 4).equals("{1to") ? Size : 0;
-  // Do not allow any other type of adjustments
-  return 0;
-}
-
 bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
                                                 OperandVector &Operands,
                                                 MCStreamer &Out,
                                                 uint64_t &ErrorInfo,
                                                 bool MatchingInlineAsm) {
   assert(!Operands.empty() && "Unexpect empty operand list!");
   X86Operand &Op = static_cast<X86Operand &>(*Operands[0]);
   assert(Op.isToken() && "Leading operand should always be a mnemonic!");
   StringRef Mnemonic = Op.getToken();
   SMRange EmptyRange = None;
   StringRef Base = Op.getToken();
 
   // First, handle aliases that expand to multiple instructions.
   MatchFPUWaitAlias(IDLoc, Op, Operands, Out, MatchingInlineAsm);
 
   MCInst Inst;
 
   // Find one unsized memory operand, if present.
   X86Operand *UnsizedMemOp = nullptr;
-  // If unsized memory operand was found - obtain following operand.
-  // For use in AdjustAVX512Mem
-  X86Operand *UnsizedMemOpNext = nullptr;
   for (const auto &Op : Operands) {
     X86Operand *X86Op = static_cast<X86Operand *>(Op.get());
-    if (UnsizedMemOp) {
-      UnsizedMemOpNext = X86Op;
+    if (X86Op->isMemUnsized()) {
+      UnsizedMemOp = X86Op;
       // Have we found an unqualified memory operand,
       // break. IA allows only one memory operand.
       break;
     }
-    if (X86Op->isMemUnsized())
-      UnsizedMemOp = X86Op;
   }
 
   // Allow some instructions to have implicitly pointer-sized operands.  This is
   // compatible with gas.
   if (UnsizedMemOp) {
     static const char *const PtrSizedInstrs[] = {"call", "jmp", "push"};
     for (const char *Instr : PtrSizedInstrs) {
       if (Mnemonic == Instr) {
for (unsigned Size : MopSizes) {
                                     MatchingInlineAsm, isParsingIntelSyntax());
       if (Match.empty() || LastOpcode != Inst.getOpcode())
         Match.push_back(M);
 
       // If this returned as a missing feature failure, remember that.
       if (Match.back() == Match_MissingFeature)
         ErrorInfoMissingFeature = ErrorInfoIgnore;
       if (M == Match_Success)
-        // MS-compatability:
-        // Adjust AVX512 vector/broadcast memory operand,
-        // when facing the absence of a size qualifier.
-        // Match GCC behavior on respective cases.
-        MatchedSize = AdjustAVX512Mem(Size, UnsizedMemOpNext);
+        // Record a successful match.
+        // We'll use it to describe the unsized memory operand, if parsing
+        // inline assembly
+        MatchedSize = Size;
     }
 
     // Restore the size of the unsized memory operand if we modified it.
     if (UnsizedMemOp)
       UnsizedMemOp->Mem.Size = 0;
   }
 
   // If we haven't matched anything yet, this is not a basic integer or FPU
bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
   }
 
   // If exactly one matched, then we treat that as a successful match (and the
   // instruction will already have been filled in correctly, since the failing
   // matches won't have modified it).
   unsigned NumSuccessfulMatches =
       std::count(std::begin(Match), std::end(Match), Match_Success);
   if (NumSuccessfulMatches == 1) {
-    if (MatchedSize && isParsingInlineAsm() && isParsingIntelSyntax())
+    if (MatchedSize && MatchingInlineAsm)
       // MS compatibility -
       // Fix the rewrite according to the matched memory size
       // MS inline assembly only
       for (AsmRewrite &AR : *InstInfo->AsmRewrites)
         if ((AR.Loc.getPointer() == UnsizedMemOp->StartLoc.getPointer()) &&
             (AR.Kind == AOK_SizeDirective))
           AR.Val = MatchedSize;
     // Some instructions need post-processing to, for example, tweak which
     // encoding is selected. Loop on it while changes happen so the individual
     // transformations can chain off each other.
     if (!MatchingInlineAsm)
       while (processInstruction(Inst, Operands))
         ;
     Inst.setLoc(IDLoc);
     if (!MatchingInlineAsm)
       EmitInstruction(Inst, Operands, Out);
     Opcode = Inst.getOpcode();
     return false;
   } else if (NumSuccessfulMatches > 1) {
     assert(UnsizedMemOp &&
            "multiple matches only possible with unsized memory operands");
     return Error(UnsizedMemOp->getStartLoc(),
                  "ambiguous operand size for instruction '" + Mnemonic + "\'",
-                 UnsizedMemOp->getLocRange(), MatchingInlineAsm);
+                 UnsizedMemOp->getLocRange());
   }
 
   // If one instruction matched with a missing feature, report this as a
   // missing feature.
   if (std::count(std::begin(Match), std::end(Match),
                  Match_MissingFeature) == 1) {
     ErrorInfo = ErrorInfoMissingFeature;
     return ErrorMissingFeature(IDLoc, ErrorInfoMissingFeature,