<div dir="ltr">It seems reasonble to me. We should also commit d0k's followup fix (r339896).<div><br></div><div>-Nirav</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 21, 2018 at 1:15 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sure.</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Tue, Aug 21, 2018 at 9:58 AM Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Someone asked on IRC about merging this to 7.0. What do you think?<br>
<br>
On Thu, Aug 16, 2018 at 9:31 AM, Nirav Dave via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: niravd<br>
> Date: Thu Aug 16 09:31:14 2018<br>
> New Revision: 339895<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=339895&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=339895&view=rev</a><br>
> Log:<br>
> [MC][X86] Enhance X86 Register expression handling to more closely match GCC.<br>
><br>
> Allow the comparison of x86 registers in the evaluation of assembler<br>
> directives. This generalizes and simplifies the extension from r334022<br>
> to catch another case found in the Linux kernel.<br>
><br>
> Reviewers: rnk, void<br>
><br>
> Reviewed By: rnk<br>
><br>
> Subscribers: hiraditya, nickdesaulniers, llvm-commits<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D50795" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D50795</a><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/MC/<wbr>MCExpr.h<br>
> llvm/trunk/include/llvm/MC/<wbr>MCParser/MCAsmParserUtils.h<br>
> llvm/trunk/include/llvm/MC/<wbr>MCParser/MCTargetAsmParser.h<br>
> llvm/trunk/lib/MC/MCExpr.cpp<br>
> llvm/trunk/lib/MC/MCParser/<wbr>AsmParser.cpp<br>
> llvm/trunk/lib/Target/X86/<wbr>AsmParser/X86AsmParser.cpp<br>
> llvm/trunk/lib/Target/X86/<wbr>MCTargetDesc/X86MCExpr.h<br>
> llvm/trunk/test/MC/X86/<wbr>pr37425.s<br>
><br>
> Modified: llvm/trunk/include/llvm/MC/<wbr>MCExpr.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCExpr.h?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/MC/MCExpr.h?rev=339895&<wbr>r1=339894&r2=339895&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/MC/<wbr>MCExpr.h (original)<br>
> +++ llvm/trunk/include/llvm/MC/<wbr>MCExpr.h Thu Aug 16 09:31:14 2018<br>
> @@ -589,6 +589,8 @@ public:<br>
> virtual bool evaluateAsRelocatableImpl(<wbr>MCValue &Res,<br>
> const MCAsmLayout *Layout,<br>
> const MCFixup *Fixup) const = 0;<br>
> + // allow Target Expressions to be checked for equality<br>
> + virtual bool isEqualTo(const MCExpr *x) const { return false; }<br>
> // This should be set when assigned expressions are not valid ".set"<br>
> // expressions, e.g. registers, and must be inlined.<br>
> virtual bool inlineAssignedExpr() const { return false; }<br>
><br>
> Modified: llvm/trunk/include/llvm/MC/<wbr>MCParser/MCAsmParserUtils.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCParser/MCAsmParserUtils.h?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/MC/MCParser/<wbr>MCAsmParserUtils.h?rev=339895&<wbr>r1=339894&r2=339895&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/MC/<wbr>MCParser/MCAsmParserUtils.h (original)<br>
> +++ llvm/trunk/include/llvm/MC/<wbr>MCParser/MCAsmParserUtils.h Thu Aug 16 09:31:14 2018<br>
> @@ -25,7 +25,7 @@ namespace MCParserUtils {<br>
> /// On success, returns false and sets the Symbol and Value output parameters.<br>
> bool parseAssignmentExpression(<wbr>StringRef Name, bool allow_redef,<br>
> MCAsmParser &Parser, MCSymbol *&Symbol,<br>
> - const MCExpr *&Value, bool AllowExtendedExpr = false);<br>
> + const MCExpr *&Value);<br>
><br>
> } // namespace MCParserUtils<br>
><br>
><br>
> Modified: llvm/trunk/include/llvm/MC/<wbr>MCParser/MCTargetAsmParser.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCParser/MCTargetAsmParser.h?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/MC/MCParser/<wbr>MCTargetAsmParser.h?rev=<wbr>339895&r1=339894&r2=339895&<wbr>view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/include/llvm/MC/<wbr>MCParser/MCTargetAsmParser.h (original)<br>
> +++ llvm/trunk/include/llvm/MC/<wbr>MCParser/MCTargetAsmParser.h Thu Aug 16 09:31:14 2018<br>
> @@ -372,9 +372,9 @@ public:<br>
> SemaCallback = Callback;<br>
> }<br>
><br>
> - // Target-specific parsing of assembler-level variable assignment.<br>
> - virtual bool parseAssignmentExpression(<wbr>const MCExpr *&Res, SMLoc &EndLoc) {<br>
> - return getParser().parseExpression(<wbr>Res, EndLoc);<br>
> + // Target-specific parsing of expression.<br>
> + virtual bool parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {<br>
> + return getParser().parsePrimaryExpr(<wbr>Res, EndLoc);<br>
> }<br>
><br>
> virtual bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc,<br>
><br>
> Modified: llvm/trunk/lib/MC/MCExpr.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCExpr.cpp?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/MC/<wbr>MCExpr.cpp?rev=339895&r1=<wbr>339894&r2=339895&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/MC/MCExpr.cpp (original)<br>
> +++ llvm/trunk/lib/MC/MCExpr.cpp Thu Aug 16 09:31:14 2018<br>
> @@ -751,8 +751,22 @@ bool MCExpr::<wbr>evaluateAsRelocatableImpl(M<br>
> if (!ABE->getLHS()-><wbr>evaluateAsRelocatableImpl(<wbr>LHSValue, Asm, Layout, Fixup,<br>
> Addrs, InSet) ||<br>
> !ABE->getRHS()-><wbr>evaluateAsRelocatableImpl(<wbr>RHSValue, Asm, Layout, Fixup,<br>
> - Addrs, InSet))<br>
> + Addrs, InSet)) {<br>
> + // Check if both are Target Expressions, see if we can compare them.<br>
> + if (const MCTargetExpr *L = dyn_cast<MCTargetExpr>(ABE-><wbr>getLHS()))<br>
> + if (const MCTargetExpr *R = cast<MCTargetExpr>(ABE-><wbr>getRHS())) {<br>
> + switch (ABE->getOpcode()) {<br>
> + case MCBinaryExpr::EQ:<br>
> + Res = MCValue::get((L->isEqualTo(R)) ? -1 : 0);<br>
> + return true;<br>
> + case MCBinaryExpr::NE:<br>
> + Res = MCValue::get((R->isEqualTo(R)) ? 0 : -1);<br>
> + return true;<br>
> + default: {}<br>
> + }<br>
> + }<br>
> return false;<br>
> + }<br>
><br>
> // We only support a few operations on non-constant expressions, handle<br>
> // those first.<br>
><br>
> Modified: llvm/trunk/lib/MC/MCParser/<wbr>AsmParser.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCParser/AsmParser.cpp?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/MC/<wbr>MCParser/AsmParser.cpp?rev=<wbr>339895&r1=339894&r2=339895&<wbr>view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/MC/MCParser/<wbr>AsmParser.cpp (original)<br>
> +++ llvm/trunk/lib/MC/MCParser/<wbr>AsmParser.cpp Thu Aug 16 09:31:14 2018<br>
> @@ -337,7 +337,7 @@ private:<br>
> StringRef parseStringToComma();<br>
><br>
> bool parseAssignment(StringRef Name, bool allow_redef,<br>
> - bool NoDeadStrip = false, bool AllowExtendedExpr = false);<br>
> + bool NoDeadStrip = false);<br>
><br>
> unsigned getBinOpPrecedence(AsmToken::<wbr>TokenKind K,<br>
> MCBinaryExpr::Opcode &Kind);<br>
> @@ -1363,7 +1363,8 @@ void AsmParser::altMacroString(<wbr>StringRef<br>
> bool AsmParser::parseExpression(<wbr>const MCExpr *&Res, SMLoc &EndLoc) {<br>
> // Parse the expression.<br>
> Res = nullptr;<br>
> - if (parsePrimaryExpr(Res, EndLoc) || parseBinOpRHS(1, Res, EndLoc))<br>
> + if (getTargetParser().<wbr>parsePrimaryExpr(Res, EndLoc) ||<br>
> + parseBinOpRHS(1, Res, EndLoc))<br>
> return true;<br>
><br>
> // As a special case, we support 'a op b @ modifier' by rewriting the<br>
> @@ -1617,7 +1618,7 @@ bool AsmParser::parseBinOpRHS(<wbr>unsigned P<br>
><br>
> // Eat the next primary expression.<br>
> const MCExpr *RHS;<br>
> - if (parsePrimaryExpr(RHS, EndLoc))<br>
> + if (getTargetParser().<wbr>parsePrimaryExpr(RHS, EndLoc))<br>
> return true;<br>
><br>
> // If BinOp binds less tightly with RHS than the operator after RHS, let<br>
> @@ -1826,7 +1827,7 @@ bool AsmParser::parseStatement(<wbr>ParseStat<br>
> // identifier '=' ... -> assignment statement<br>
> Lex();<br>
><br>
> - return parseAssignment(IDVal, true, /*NoDeadStrip*/ false, /*AllowExtendedExpr*/true);<br>
> + return parseAssignment(IDVal, true);<br>
><br>
> default: // Normal instruction or directive.<br>
> break;<br>
> @@ -2766,11 +2767,11 @@ void AsmParser::handleMacroExit() {<br>
> }<br>
><br>
> bool AsmParser::parseAssignment(<wbr>StringRef Name, bool allow_redef,<br>
> - bool NoDeadStrip, bool AllowExtendedExpr) {<br>
> + bool NoDeadStrip) {<br>
> MCSymbol *Sym;<br>
> const MCExpr *Value;<br>
> if (MCParserUtils::<wbr>parseAssignmentExpression(<wbr>Name, allow_redef, *this, Sym,<br>
> - Value, AllowExtendedExpr))<br>
> + Value))<br>
> return true;<br>
><br>
> if (!Sym) {<br>
> @@ -5839,17 +5840,13 @@ static bool isSymbolUsedInExpression(con<br>
><br>
> bool parseAssignmentExpression(<wbr>StringRef Name, bool allow_redef,<br>
> MCAsmParser &Parser, MCSymbol *&Sym,<br>
> - const MCExpr *&Value, bool AllowExtendedExpr) {<br>
> + const MCExpr *&Value) {<br>
><br>
> // FIXME: Use better location, we should use proper tokens.<br>
> SMLoc EqualLoc = Parser.getTok().getLoc();<br>
> SMLoc EndLoc;<br>
> - if (AllowExtendedExpr) {<br>
> - if (Parser.getTargetParser().<wbr>parseAssignmentExpression(<wbr>Value, EndLoc)) {<br>
> - return Parser.TokError("missing expression");<br>
> - }<br>
> - } else if (Parser.parseExpression(Value, EndLoc))<br>
> - return Parser.TokError("missing expression");<br>
> + if (Parser.parseExpression(Value)<wbr>)<br>
> + return Parser.TokError("missing expression");<br>
><br>
> // Note: we don't count b as used in "a = b". This is to allow<br>
> // a = b<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=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>X86/AsmParser/X86AsmParser.<wbr>cpp?rev=339895&r1=339894&r2=<wbr>339895&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 Aug 16 09:31:14 2018<br>
> @@ -955,7 +955,7 @@ public:<br>
><br>
> void SetFrameRegister(unsigned RegNo) override;<br>
><br>
> - bool parseAssignmentExpression(<wbr>const MCExpr *&Res, SMLoc &EndLoc) override;<br>
> + bool parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) override;<br>
><br>
> bool ParseInstruction(<wbr>ParseInstructionInfo &Info, StringRef Name,<br>
> SMLoc NameLoc, OperandVector &Operands) override;<br>
> @@ -2260,15 +2260,17 @@ std::unique_ptr<X86Operand> X86AsmParser<br>
> return X86Operand::CreateMem(<wbr>getPointerWidth(), Disp, MemStart, MemEnd);<br>
> }<br>
><br>
> -// Parse either a standard expression or a register.<br>
> -bool X86AsmParser::<wbr>parseAssignmentExpression(<wbr>const MCExpr *&Res,<br>
> - SMLoc &EndLoc) {<br>
> +// Parse either a standard primary expression or a register.<br>
> +bool X86AsmParser::<wbr>parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {<br>
> MCAsmParser &Parser = getParser();<br>
> - if (Parser.parseExpression(Res, EndLoc)) {<br>
> + if (Parser.parsePrimaryExpr(Res, EndLoc)) {<br>
> SMLoc StartLoc = Parser.getTok().getLoc();<br>
> // Normal Expression parse fails, check if it could be a register.<br>
> unsigned RegNo;<br>
> - if (Parser.getTargetParser().<wbr>ParseRegister(RegNo, StartLoc, EndLoc))<br>
> + bool TryRegParse =<br>
> + getTok().is(AsmToken::Percent) ||<br>
> + (isParsingIntelSyntax() && getTok().is(AsmToken::<wbr>Identifier));<br>
> + if (!TryRegParse || ParseRegister(RegNo, StartLoc, EndLoc))<br>
> return true;<br>
> // Clear previous parse error and return correct expression.<br>
> Parser.clearPendingErrors();<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/<wbr>MCTargetDesc/X86MCExpr.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/MCTargetDesc/X86MCExpr.h?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>X86/MCTargetDesc/X86MCExpr.h?<wbr>rev=339895&r1=339894&r2=<wbr>339895&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Target/X86/<wbr>MCTargetDesc/X86MCExpr.h (original)<br>
> +++ llvm/trunk/lib/Target/X86/<wbr>MCTargetDesc/X86MCExpr.h Thu Aug 16 09:31:14 2018<br>
> @@ -48,7 +48,7 @@ public:<br>
> /// @}<br>
><br>
> void printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const override {<br>
> - if (MAI->getAssemblerDialect() == 0)<br>
> + if (!MAI || MAI->getAssemblerDialect() == 0)<br>
> OS << '%';<br>
> OS << X86ATTInstPrinter::<wbr>getRegisterName(RegNo);<br>
> }<br>
> @@ -59,6 +59,11 @@ public:<br>
> }<br>
> // Register values should be inlined as they are not valid .set expressions.<br>
> bool inlineAssignedExpr() const override { return true; }<br>
> + bool isEqualTo(const MCExpr *X) const override {<br>
> + if (auto *E = dyn_cast<X86MCExpr>(X))<br>
> + return getRegNo() == E->getRegNo();<br>
> + return false;<br>
> + }<br>
> void visitUsedExpr(MCStreamer &Streamer) const override{};<br>
> MCFragment *findAssociatedFragment() const override { return nullptr; }<br>
><br>
><br>
> Modified: llvm/trunk/test/MC/X86/<wbr>pr37425.s<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/X86/pr37425.s?rev=339895&r1=339894&r2=339895&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/MC/<wbr>X86/pr37425.s?rev=339895&r1=<wbr>339894&r2=339895&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/test/MC/X86/<wbr>pr37425.s (original)<br>
> +++ llvm/trunk/test/MC/X86/<wbr>pr37425.s Thu Aug 16 09:31:14 2018<br>
> @@ -1,5 +1,4 @@<br>
> -// RUN: llvm-mc -triple x86_64-unknown-unknown -defsym=ERR=0 %s -o - | FileCheck %s<br>
> -// RUN: not llvm-mc -triple x86_64-unknown-unknown -defsym=ERR=1 %s -o - 2>&1 | FileCheck --check-prefix=ERR %s<br>
> +// RUN: llvm-mc -triple x86_64-unknown-unknown %s -o - | FileCheck %s<br>
><br>
> // CHECK-NOT: .set var_xdata<br>
> var_xdata = %rcx<br>
> @@ -7,10 +6,15 @@ var_xdata = %rcx<br>
> // CHECK: xorq %rcx, %rcx<br>
> xorq var_xdata, var_xdata<br>
><br>
> -.if (ERR==1)<br>
> -// ERR: [[@LINE+2]]:15: error: unknown token in expression in '.set' directive<br>
> -// ERR: [[@LINE+1]]:15: error: missing expression in '.set' directive<br>
> -.set err_var, %rcx<br>
> -.endif<br>
> +// CHECK: .data<br>
> +// CHECK-NEXT: .byte 1<br>
> +.data<br>
> +.if var_xdata == %rax<br>
> + .byte 0<br>
> +.elseif var_xdata == %rcx<br>
> + .byte 1<br>
> +.else<br>
> + .byte 2<br>
> +.endif<br>
><br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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>
</blockquote></div>
</div></div></blockquote></div><br></div>