Skip to content

Commit

Permalink
[ms-inline-asm] Use the frontend size only for ambiguous instructions
Browse files Browse the repository at this point in the history 
This avoids problems on code like this:
  char buf[16];
  __asm {
    movups xmm0, [buf]
    mov [buf], eax
  }

The frontend size in this case (1) is wrong, and the register makes the
instruction matching unambiguous. There are also enough bytes available
that we shouldn't complain to the user that they are potentially using
an incorrectly sized instruction to access the variable.

Supersedes D32636 and D26586 and fixes PR28266

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@302179 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@rnk
rnk committed May 4, 2017
1 parent b6a6182 commit 984dc04
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 66 deletions.
94 changes: 29 additions & 65 deletions lib/Target/X86/AsmParser/X86AsmParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -776,11 +776,6 @@ class X86AsmParser : public MCTargetAsmParser {

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];
Expand Down Expand Up @@ -1206,35 +1201,24 @@ std::unique_ptr<X86Operand> X86AsmParser::CreateMemForInlineAsm(
Identifier, Info.OpDecl);
}


// We either have a direct symbol reference, or an offset from a symbol. The
// parser always puts the symbol on the LHS, so look there for size
// calculation purposes.
unsigned FrontendSize = 0;
const MCBinaryExpr *BinOp = dyn_cast<MCBinaryExpr>(Disp);
bool IsSymRef =
isa<MCSymbolRefExpr>(BinOp ? BinOp->getLHS() : Disp);
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]"
// 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;
}
}
if (IsSymRef && !Size && Info.Type)
FrontendSize = Info.Type * 8; // Size is in terms of bits in this context.

// When parsing inline assembly we set the base register to a non-zero value
// if we don't know the actual value at this time. This is necessary to
// get the matching correct in some cases.
BaseReg = BaseReg ? BaseReg : 1;
return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
IndexReg, Scale, Start, End, Size, Identifier,
Info.OpDecl);
Info.OpDecl, FrontendSize);
}

static void
Expand Down Expand Up @@ -2884,23 +2868,6 @@ bool X86AsmParser::MatchAndEmitATTInstruction(SMLoc IDLoc, unsigned &Opcode,
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,
Expand All @@ -2920,19 +2887,14 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,

// 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
Expand Down Expand Up @@ -2978,7 +2940,6 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
// If an unsized memory operand is present, try to match with each memory
// operand size. In Intel assembly, the size is not part of the instruction
// mnemonic.
unsigned MatchedSize = 0;
if (UnsizedMemOp && UnsizedMemOp->isMemUnsized()) {
static const unsigned MopSizes[] = {8, 16, 32, 64, 80, 128, 256, 512};
for (unsigned Size : MopSizes) {
Expand All @@ -2993,17 +2954,10 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
// 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);
}

// Restore the size of the unsized memory operand if we modified it.
if (UnsizedMemOp)
UnsizedMemOp->Mem.Size = 0;
UnsizedMemOp->Mem.Size = 0;
}

// If we haven't matched anything yet, this is not a basic integer or FPU
Expand All @@ -3027,20 +2981,30 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
Op.getLocRange(), MatchingInlineAsm);
}

unsigned NumSuccessfulMatches =
std::count(std::begin(Match), std::end(Match), Match_Success);

// If matching was ambiguous and we had size information from the frontend,
// try again with that. This handles cases like "movxz eax, m8/m16".
if (UnsizedMemOp && NumSuccessfulMatches > 1 &&
UnsizedMemOp->getMemFrontendSize()) {
UnsizedMemOp->Mem.Size = UnsizedMemOp->getMemFrontendSize();
unsigned M = MatchInstruction(
Operands, Inst, ErrorInfo, MatchingInlineAsm, isParsingIntelSyntax());
if (M == Match_Success)
NumSuccessfulMatches = 1;

// Add a rewrite that encodes the size information we used from the
// frontend.
InstInfo->AsmRewrites->emplace_back(
AOK_SizeDirective, UnsizedMemOp->getStartLoc(),
/*Len=*/0, UnsizedMemOp->getMemFrontendSize());
}

// 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())
// 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.
Expand All @@ -3057,7 +3021,7 @@ bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
"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
Expand Down
11 changes: 10 additions & 1 deletion lib/Target/X86/AsmParser/X86Operand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ struct X86Operand : public MCParsedAsmOperand {
unsigned Scale;
unsigned Size;
unsigned ModeSize;

/// If the memory operand is unsized and there are multiple instruction
/// matches, prefer the one with this size.
unsigned FrontendSize;
};

union {
Expand Down Expand Up @@ -136,6 +140,10 @@ struct X86Operand : public MCParsedAsmOperand {
assert(Kind == Memory && "Invalid access!");
return Mem.ModeSize;
}
unsigned getMemFrontendSize() const {
assert(Kind == Memory && "Invalid access!");
return Mem.FrontendSize;
}

bool isToken() const override {return Kind == Token; }

Expand Down Expand Up @@ -532,7 +540,7 @@ struct X86Operand : public MCParsedAsmOperand {
CreateMem(unsigned ModeSize, unsigned SegReg, const MCExpr *Disp,
unsigned BaseReg, unsigned IndexReg, unsigned Scale, SMLoc StartLoc,
SMLoc EndLoc, unsigned Size = 0, StringRef SymName = StringRef(),
void *OpDecl = nullptr) {
void *OpDecl = nullptr, unsigned FrontendSize = 0) {
// We should never just have a displacement, that should be parsed as an
// absolute memory operand.
assert((SegReg || BaseReg || IndexReg) && "Invalid memory operand!");
Expand All @@ -548,6 +556,7 @@ struct X86Operand : public MCParsedAsmOperand {
Res->Mem.Scale = Scale;
Res->Mem.Size = Size;
Res->Mem.ModeSize = ModeSize;
Res->Mem.FrontendSize = FrontendSize;
Res->SymName = SymName;
Res->OpDecl = OpDecl;
Res->AddressOf = false;
Expand Down
24 changes: 24 additions & 0 deletions test/CodeGen/X86/ms-inline-asm-avx512.ll
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
; RUN: llc < %s | FileCheck %s

; Generated from clang/test/CodeGen/ms-inline-asm-avx512.c

target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"

; Function Attrs: noinline nounwind
define void @ignore_fe_size() #0 {
entry:
%c = alloca i8, align 1
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},~{dirflag},~{fpsr},~{flags}"(i8* %c, i8* %c, i8* %c, i8* %c) #1
ret void
}

; CHECK-LABEL: ignore_fe_size:
; CHECK: vaddps 7(%rsp){1to4}, %xmm2, %xmm1
; CHECK: vaddps 7(%rsp), %xmm2, %xmm1
; CHECK: movl 7(%rsp), %eax
; CHECK: movq %rax, 7(%rsp)
; CHECK: retq

attributes #0 = { noinline nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="skylake-avx512" "target-features"="+adx,+aes,+avx,+avx2,+avx512bw,+avx512cd,+avx512dq,+avx512f,+avx512vl,+bmi,+bmi2,+clflushopt,+clwb,+cx16,+f16c,+fma,+fsgsbase,+fxsr,+lzcnt,+mmx,+movbe,+mpx,+pclmul,+pku,+popcnt,+rdrnd,+rdseed,+rtm,+sgx,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsavec,+xsaveopt,+xsaves" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { nounwind }

0 comments on commit 984dc04

Please sign in to comment.