[PATCH] D26586: [AVX512][inline-asm] Fix AVX512 inline assembly instruction resolution when the size qualifier of a memory operand is not specified explicitly.

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 4 08:43:38 PDT 2017


rnk added a comment.

I feel like we need to step back and ask why we are using the variable size information provided by the frontend at all. I don't think MSVC uses it. We probably only needed it because our intel asm matcher couldn't cope with ambiguity before and it was getting the wrong sizes.

I'm going to look into removing the frontend-directed AOK_SizeDirective rewrite altogether.



================
Comment at: llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp:1208
       if (Size)
         InstInfo->AsmRewrites->emplace_back(AOK_SizeDirective, Start,
                                             /*Len=*/0, Size);
----------------
It seems ridiculous to emit this size directive modification here and then rewrite it later in the asm matcher. Surely there is a better way to do this.


================
Comment at: llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp:1210
                                             /*Len=*/0, Size);
+    if (AllowBetterSizeMatch)
+      // Handle cases where size qualifier is absent, upon an indirect symbol
----------------
This code is not properly indented, and this actually confused me while reading the committed code.


================
Comment at: llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp:1213
+      // 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
----------------
typo on "mechanism"


================
Comment at: llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp:1504
+                               End, Size, SM.getSymName(), Info,
+                               isParsingInlineAsm());
 }
----------------
We are always parsing inline asm when calling `CreateMemForInlineAsm`, that's why we're here.


================
Comment at: llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp:2845
 
+unsigned X86AsmParser::AdjustAVX512Mem(unsigned Size,
+    X86Operand* UnsizedMemOpNext) {
----------------
This seems unnecessary, which makes sense because your follow-up change D32636 removes it


Repository:
  rL LLVM

https://reviews.llvm.org/D26586





More information about the llvm-commits mailing list