<div dir="ltr">Sorry about that. Daniel Jasper initialized FrontendSize in r302214.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 4, 2017 at 4:31 PM, Evgenii Stepanov <span dir="ltr"><<a href="mailto:eugeni.stepanov@gmail.com" target="_blank">eugeni.stepanov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
MSan bot is not happy with this change:<br>
<a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/4693" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/<wbr>builders/sanitizer-x86_64-<wbr>linux-fast/builds/4693</a><br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On Thu, May 4, 2017 at 11:19 AM, Reid Kleckner via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: rnk<br>
> Date: Thu May 4 13:19:52 2017<br>
> New Revision: 302179<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=302179&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=302179&view=rev</a><br>
> Log:<br>
> [ms-inline-asm] Use the frontend size only for ambiguous instructions<br>
><br>
> This avoids problems on code like this:<br>
> char buf[16];<br>
> __asm {<br>
> movups xmm0, [buf]<br>
> mov [buf], eax<br>
> }<br>
><br>
> The frontend size in this case (1) is wrong, and the register makes the<br>
> instruction matching unambiguous. There are also enough bytes available<br>
> that we shouldn't complain to the user that they are potentially using<br>
> an incorrectly sized instruction to access the variable.<br>
><br>
> Supersedes D32636 and D26586 and fixes PR28266<br>
><br>
> Added:<br>
> llvm/trunk/test/CodeGen/X86/<wbr>ms-inline-asm-avx512.ll<br>
> Modified:<br>
> llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86AsmParser.cpp<br>
> llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86Operand.h<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86AsmParser.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp?rev=302179&r1=302178&r2=302179&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>X86/AsmParser/X86AsmParser.<wbr>cpp?rev=302179&r1=302178&r2=<wbr>302179&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86AsmParser.cpp (original)<br>
> +++ llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86AsmParser.cpp Thu May 4 13:19:52 2017<br>
> @@ -776,11 +776,6 @@ private:<br>
><br>
> bool ParseZ(std::unique_ptr<<wbr>X86Operand> &Z, const SMLoc &StartLoc);<br>
><br>
> - /// MS-compatibility:<br>
> - /// Obtain an appropriate size qualifier, when facing its absence,<br>
> - /// upon AVX512 vector/broadcast memory operand<br>
> - unsigned AdjustAVX512Mem(unsigned Size, X86Operand* UnsizedMemOpNext);<br>
> -<br>
> bool is64BitMode() const {<br>
> // FIXME: Can tablegen auto-generate this?<br>
> return getSTI().getFeatureBits()[X86:<wbr>:Mode64Bit];<br>
> @@ -1206,27 +1201,16 @@ std::unique_ptr<X86Operand> X86AsmParser<br>
> Identifier, Info.OpDecl);<br>
> }<br>
><br>
> +<br>
> // We either have a direct symbol reference, or an offset from a symbol. The<br>
> // parser always puts the symbol on the LHS, so look there for size<br>
> // calculation purposes.<br>
> + unsigned FrontendSize = 0;<br>
> const MCBinaryExpr *BinOp = dyn_cast<MCBinaryExpr>(Disp);<br>
> bool IsSymRef =<br>
> isa<MCSymbolRefExpr>(BinOp ? BinOp->getLHS() : Disp);<br>
> - if (IsSymRef) {<br>
> - if (!Size) {<br>
> - Size = Info.Type * 8; // Size is in terms of bits in this context.<br>
> - if (Size)<br>
> - InstInfo->AsmRewrites-><wbr>emplace_back(AOK_<wbr>SizeDirective, Start,<br>
> - /*Len=*/0, Size);<br>
> - if (AllowBetterSizeMatch)<br>
> - // Handle cases where size qualifier is absent, upon an indirect symbol<br>
> - // reference - e.g. "vaddps zmm1, zmm2, [var]"<br>
> - // set Size to zero to allow matching mechansim to try and find a better<br>
> - // size qualifier than our initial guess, based on available variants of<br>
> - // the given instruction<br>
> - Size = 0;<br>
> - }<br>
> - }<br>
> + if (IsSymRef && !Size && Info.Type)<br>
> + FrontendSize = Info.Type * 8; // Size is in terms of bits in this context.<br>
><br>
> // When parsing inline assembly we set the base register to a non-zero value<br>
> // if we don't know the actual value at this time. This is necessary to<br>
> @@ -1234,7 +1218,7 @@ std::unique_ptr<X86Operand> X86AsmParser<br>
> BaseReg = BaseReg ? BaseReg : 1;<br>
> return X86Operand::CreateMem(<wbr>getPointerWidth(), SegReg, Disp, BaseReg,<br>
> IndexReg, Scale, Start, End, Size, Identifier,<br>
> - Info.OpDecl);<br>
> + Info.OpDecl, FrontendSize);<br>
> }<br>
><br>
> static void<br>
> @@ -2884,23 +2868,6 @@ bool X86AsmParser::<wbr>MatchAndEmitATTInstru<br>
> return true;<br>
> }<br>
><br>
> -unsigned X86AsmParser::AdjustAVX512Mem(<wbr>unsigned Size,<br>
> - X86Operand* UnsizedMemOpNext) {<br>
> - // Check for the existence of an AVX512 platform<br>
> - if (!getSTI().getFeatureBits()[<wbr>X86::FeatureAVX512])<br>
> - return 0;<br>
> - // Allow adjusting upon a (x|y|z)mm<br>
> - if (Size == 512 || Size == 256 || Size == 128)<br>
> - return Size;<br>
> - // This is an allegadly broadcasting mem op adjustment,<br>
> - // allow some more inquiring to validate it<br>
> - if (Size == 64 || Size == 32)<br>
> - return UnsizedMemOpNext && UnsizedMemOpNext->isToken() &&<br>
> - UnsizedMemOpNext->getToken().<wbr>substr(0, 4).equals("{1to") ? Size : 0;<br>
> - // Do not allow any other type of adjustments<br>
> - return 0;<br>
> -}<br>
> -<br>
> bool X86AsmParser::<wbr>MatchAndEmitIntelInstruction(<wbr>SMLoc IDLoc, unsigned &Opcode,<br>
> OperandVector &Operands,<br>
> MCStreamer &Out,<br>
> @@ -2920,19 +2887,14 @@ bool X86AsmParser::<wbr>MatchAndEmitIntelInst<br>
><br>
> // Find one unsized memory operand, if present.<br>
> X86Operand *UnsizedMemOp = nullptr;<br>
> - // If unsized memory operand was found - obtain following operand.<br>
> - // For use in AdjustAVX512Mem<br>
> - X86Operand *UnsizedMemOpNext = nullptr;<br>
> for (const auto &Op : Operands) {<br>
> X86Operand *X86Op = static_cast<X86Operand *>(Op.get());<br>
> - if (UnsizedMemOp) {<br>
> - UnsizedMemOpNext = X86Op;<br>
> + if (X86Op->isMemUnsized()) {<br>
> + UnsizedMemOp = X86Op;<br>
> // Have we found an unqualified memory operand,<br>
> // break. IA allows only one memory operand.<br>
> break;<br>
> }<br>
> - if (X86Op->isMemUnsized())<br>
> - UnsizedMemOp = X86Op;<br>
> }<br>
><br>
> // Allow some instructions to have implicitly pointer-sized operands. This is<br>
> @@ -2978,7 +2940,6 @@ bool X86AsmParser::<wbr>MatchAndEmitIntelInst<br>
> // If an unsized memory operand is present, try to match with each memory<br>
> // operand size. In Intel assembly, the size is not part of the instruction<br>
> // mnemonic.<br>
> - unsigned MatchedSize = 0;<br>
> if (UnsizedMemOp && UnsizedMemOp->isMemUnsized()) {<br>
> static const unsigned MopSizes[] = {8, 16, 32, 64, 80, 128, 256, 512};<br>
> for (unsigned Size : MopSizes) {<br>
> @@ -2993,17 +2954,10 @@ bool X86AsmParser::<wbr>MatchAndEmitIntelInst<br>
> // If this returned as a missing feature failure, remember that.<br>
> if (Match.back() == Match_MissingFeature)<br>
> ErrorInfoMissingFeature = ErrorInfoIgnore;<br>
> - if (M == Match_Success)<br>
> - // MS-compatability:<br>
> - // Adjust AVX512 vector/broadcast memory operand,<br>
> - // when facing the absence of a size qualifier.<br>
> - // Match GCC behavior on respective cases.<br>
> - MatchedSize = AdjustAVX512Mem(Size, UnsizedMemOpNext);<br>
> }<br>
><br>
> // Restore the size of the unsized memory operand if we modified it.<br>
> - if (UnsizedMemOp)<br>
> - UnsizedMemOp->Mem.Size = 0;<br>
> + UnsizedMemOp->Mem.Size = 0;<br>
> }<br>
><br>
> // If we haven't matched anything yet, this is not a basic integer or FPU<br>
> @@ -3027,20 +2981,30 @@ bool X86AsmParser::<wbr>MatchAndEmitIntelInst<br>
> Op.getLocRange(), MatchingInlineAsm);<br>
> }<br>
><br>
> + unsigned NumSuccessfulMatches =<br>
> + std::count(std::begin(Match), std::end(Match), Match_Success);<br>
> +<br>
> + // If matching was ambiguous and we had size information from the frontend,<br>
> + // try again with that. This handles cases like "movxz eax, m8/m16".<br>
> + if (UnsizedMemOp && NumSuccessfulMatches > 1 &&<br>
> + UnsizedMemOp-><wbr>getMemFrontendSize()) {<br>
> + UnsizedMemOp->Mem.Size = UnsizedMemOp-><wbr>getMemFrontendSize();<br>
> + unsigned M = MatchInstruction(<br>
> + Operands, Inst, ErrorInfo, MatchingInlineAsm, isParsingIntelSyntax());<br>
> + if (M == Match_Success)<br>
> + NumSuccessfulMatches = 1;<br>
> +<br>
> + // Add a rewrite that encodes the size information we used from the<br>
> + // frontend.<br>
> + InstInfo->AsmRewrites-><wbr>emplace_back(<br>
> + AOK_SizeDirective, UnsizedMemOp->getStartLoc(),<br>
> + /*Len=*/0, UnsizedMemOp-><wbr>getMemFrontendSize());<br>
> + }<br>
> +<br>
> // If exactly one matched, then we treat that as a successful match (and the<br>
> // instruction will already have been filled in correctly, since the failing<br>
> // matches won't have modified it).<br>
> - unsigned NumSuccessfulMatches =<br>
> - std::count(std::begin(Match), std::end(Match), Match_Success);<br>
> if (NumSuccessfulMatches == 1) {<br>
> - if (MatchedSize && isParsingInlineAsm() && isParsingIntelSyntax())<br>
> - // MS compatibility -<br>
> - // Fix the rewrite according to the matched memory size<br>
> - // MS inline assembly only<br>
> - for (AsmRewrite &AR : *InstInfo->AsmRewrites)<br>
> - if ((AR.Loc.getPointer() == UnsizedMemOp->StartLoc.<wbr>getPointer()) &&<br>
> - (AR.Kind == AOK_SizeDirective))<br>
> - AR.Val = MatchedSize;<br>
> // Some instructions need post-processing to, for example, tweak which<br>
> // encoding is selected. Loop on it while changes happen so the individual<br>
> // transformations can chain off each other.<br>
> @@ -3057,7 +3021,7 @@ bool X86AsmParser::<wbr>MatchAndEmitIntelInst<br>
> "multiple matches only possible with unsized memory operands");<br>
> return Error(UnsizedMemOp-><wbr>getStartLoc(),<br>
> "ambiguous operand size for instruction '" + Mnemonic + "\'",<br>
> - UnsizedMemOp->getLocRange(), MatchingInlineAsm);<br>
> + UnsizedMemOp->getLocRange());<br>
> }<br>
><br>
> // If one instruction matched with a missing feature, report this as a<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86Operand.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/AsmParser/X86Operand.h?rev=302179&r1=302178&r2=302179&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>X86/AsmParser/X86Operand.h?<wbr>rev=302179&r1=302178&r2=<wbr>302179&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86Operand.h (original)<br>
> +++ llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86Operand.h Thu May 4 13:19:52 2017<br>
> @@ -62,6 +62,10 @@ struct X86Operand : public MCParsedAsmOp<br>
> unsigned Scale;<br>
> unsigned Size;<br>
> unsigned ModeSize;<br>
> +<br>
> + /// If the memory operand is unsized and there are multiple instruction<br>
> + /// matches, prefer the one with this size.<br>
> + unsigned FrontendSize;<br>
> };<br>
><br>
> union {<br>
> @@ -136,6 +140,10 @@ struct X86Operand : public MCParsedAsmOp<br>
> assert(Kind == Memory && "Invalid access!");<br>
> return Mem.ModeSize;<br>
> }<br>
> + unsigned getMemFrontendSize() const {<br>
> + assert(Kind == Memory && "Invalid access!");<br>
> + return Mem.FrontendSize;<br>
> + }<br>
><br>
> bool isToken() const override {return Kind == Token; }<br>
><br>
> @@ -532,7 +540,7 @@ struct X86Operand : public MCParsedAsmOp<br>
> CreateMem(unsigned ModeSize, unsigned SegReg, const MCExpr *Disp,<br>
> unsigned BaseReg, unsigned IndexReg, unsigned Scale, SMLoc StartLoc,<br>
> SMLoc EndLoc, unsigned Size = 0, StringRef SymName = StringRef(),<br>
> - void *OpDecl = nullptr) {<br>
> + void *OpDecl = nullptr, unsigned FrontendSize = 0) {<br>
> // We should never just have a displacement, that should be parsed as an<br>
> // absolute memory operand.<br>
> assert((SegReg || BaseReg || IndexReg) && "Invalid memory operand!");<br>
> @@ -548,6 +556,7 @@ struct X86Operand : public MCParsedAsmOp<br>
> Res->Mem.Scale = Scale;<br>
> Res->Mem.Size = Size;<br>
> Res->Mem.ModeSize = ModeSize;<br>
> + Res->Mem.FrontendSize = FrontendSize;<br>
> Res->SymName = SymName;<br>
> Res->OpDecl = OpDecl;<br>
> Res->AddressOf = false;<br>
><br>
> Added: llvm/trunk/test/CodeGen/X86/<wbr>ms-inline-asm-avx512.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/ms-inline-asm-avx512.ll?rev=302179&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>CodeGen/X86/ms-inline-asm-<wbr>avx512.ll?rev=302179&view=auto</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/test/CodeGen/X86/<wbr>ms-inline-asm-avx512.ll (added)<br>
> +++ llvm/trunk/test/CodeGen/X86/<wbr>ms-inline-asm-avx512.ll Thu May 4 13:19:52 2017<br>
> @@ -0,0 +1,24 @@<br>
> +; RUN: llc < %s | FileCheck %s<br>
> +<br>
> +; Generated from clang/test/CodeGen/ms-inline-<wbr>asm-avx512.c<br>
> +<br>
> +target datalayout = "e-m:w-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
> +target triple = "x86_64-pc-windows-msvc"<br>
> +<br>
> +; Function Attrs: noinline nounwind<br>
> +define void @ignore_fe_size() #0 {<br>
> +entry:<br>
> + %c = alloca i8, align 1<br>
> + call void asm sideeffect inteldialect "vaddps xmm1, xmm2, $1{1to4}\0A\09vaddps xmm1, xmm2, $2\0A\09mov eax, $3\0A\09mov $0, rax", "=*m,*m,*m,*m,~{eax},~{xmm1},~<wbr>{dirflag},~{fpsr},~{flags}"(<wbr>i8* %c, i8* %c, i8* %c, i8* %c) #1<br>
> + ret void<br>
> +}<br>
> +<br>
> +; CHECK-LABEL: ignore_fe_size:<br>
> +; CHECK: vaddps 7(%rsp){1to4}, %xmm2, %xmm1<br>
> +; CHECK: vaddps 7(%rsp), %xmm2, %xmm1<br>
> +; CHECK: movl 7(%rsp), %eax<br>
> +; CHECK: movq %rax, 7(%rsp)<br>
> +; CHECK: retq<br>
> +<br>
> +attributes #0 = { noinline nounwind "correctly-rounded-divide-<wbr>sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="<wbr>false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="<wbr>false" "no-trapping-math"="false" "stack-protector-buffer-size"=<wbr>"8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+<wbr>avx,+avx2,+avx512bw,+avx512cd,<wbr>+avx512dq,+avx512f,+avx512vl,+<wbr>bmi,+bmi2,+clflushopt,+clwb,+<wbr>cx16,+f16c,+fma,+fsgsbase,+<wbr>fxsr,+lzcnt,+mmx,+movbe,+mpx,+<wbr>pclmul,+pku,+popcnt,+rdrnd,+<wbr>rdseed,+rtm,+sgx,+sse,+sse2,+<wbr>sse3,+sse4.1,+sse4.2,+ssse3,+<wbr>x87,+xsave,+xsavec,+xsaveopt,+<wbr>xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }<br>
> +attributes #1 = { nounwind }<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>