[llvm-commits] [llvm] r123292 - in /llvm/trunk: lib/Target/ARM/AsmParser/ARMAsmParser.cpp test/MC/ARM/elf-movt.s
Jason Kim
jasonwkim at google.com
Wed Jan 12 14:04:19 PST 2011
On Wed, Jan 12, 2011 at 1:44 PM, Jim Grosbach <grosbach at apple.com> wrote:
> Leaving aside most architectural questions as they're be subsumed by the changes to have the variantkind stuff apply to more than just symbolref expressions. A few comments below.
>
>
> On Jan 11, 2011, at 3:53 PM, Jason W Kim wrote:
>
>> Author: jasonwkim
>> Date: Tue Jan 11 17:53:41 2011
>> New Revision: 123292
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=123292&view=rev
>> Log:
>> Workaround for bug 8721.
>> .s Test added.
>>
>>
>> Added:
>> llvm/trunk/test/MC/ARM/elf-movt.s
>> Modified:
>> llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
>>
>> Modified: llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp?rev=123292&r1=123291&r2=123292&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp Tue Jan 11 17:53:41 2011
>> @@ -55,6 +55,10 @@
>> bool ParseRegisterList(SmallVectorImpl<MCParsedAsmOperand*> &);
>> bool ParseMemory(SmallVectorImpl<MCParsedAsmOperand*> &);
>> bool ParseOperand(SmallVectorImpl<MCParsedAsmOperand*> &);
>> + bool ParsePrefix(MCSymbolRefExpr::VariantKind &RefKind);
>> + const MCExpr *ApplyPrefixToExpr(const MCExpr *E,
>> + MCSymbolRefExpr::VariantKind Variant);
>> +
>>
>> bool ParseMemoryOffsetReg(bool &Negative,
>> bool &OffsetRegShifted,
>> @@ -864,9 +868,111 @@
>> E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
>> Operands.push_back(ARMOperand::CreateImm(ImmVal, S, E));
>> return false;
>> + case AsmToken::Colon: {
>> + // ":lower16:" and ":upper16:" expression prefixes
>> + MCSymbolRefExpr::VariantKind RefKind;
>> + if (ParsePrefix(RefKind))
>> + return true;
>> +
>> + const MCExpr *ExprVal;
>> + if (getParser().ParseExpression(ExprVal))
>> + return true;
>> +
>> + // TODO: Attach the prefix to the entire expression
>> + // instead of just the first symbol.
>> + const MCExpr *ModExprVal = ApplyPrefixToExpr(ExprVal, RefKind);
>> + if (!ModExprVal) {
>> + return TokError("invalid modifier '" + getTok().getIdentifier() +
>> + "' (no symbols present)");
>> + }
>> +
>> + E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
>> + Operands.push_back(ARMOperand::CreateImm(ModExprVal, S, E));
>> + return false;
>> + }
>> }
>> }
>>
>> +// FIXME: The next 2 routines are hacks to get ARMAsmParser to understand
>> +// :lower16: and :upper16:
>> +// It still attaches VK_ARM_HI/LO16 to MCSymbolRefExpr, but it really
>> +// should be attached to the entire MCExpr as a whole - perhaps using
>> +// MCTargetExpr?
>> +bool ARMAsmParser::ParsePrefix(MCSymbolRefExpr::VariantKind &RefKind) {
>> + RefKind = MCSymbolRefExpr::VK_None;
>> +
>> + // :lower16: and :upper16: modifiers
>> + if (getLexer().isNot(AsmToken::Colon)) {
>> + Error(Parser.getTok().getLoc(), "expected :");
>> + return true;
>> + }
>
> This can just be an assert(), not an error. This is only called when a ':' token has been detected, else we have a parser bug and want to abort.
Okay. I will fix this.
>
>> + Parser.Lex(); // Eat ':'
>> +
>> + if (getLexer().isNot(AsmToken::Identifier)) {
>> + Error(Parser.getTok().getLoc(), "expected prefix identifier in operand");
>> + return true;
>> + }
>> +
>> + StringRef IDVal = Parser.getTok().getIdentifier();
>> + if (IDVal == "lower16") {
>> + RefKind = MCSymbolRefExpr::VK_ARM_LO16;
>> + } else if (IDVal == "upper16") {
>> + RefKind = MCSymbolRefExpr::VK_ARM_HI16;
>> + } else {
>> + Error(Parser.getTok().getLoc(), "unexpected prefix in operand");
>> + return true;
>> + }
>> + Parser.Lex();
>> +
>> + if (getLexer().isNot(AsmToken::Colon)) {
>> + Error(Parser.getTok().getLoc(), "unexpected token after prefix");
>> + return true;
>> + }
>> + Parser.Lex(); // Eat the last ':'
>> + return false;
>> +}
>> +
>> +const MCExpr *
>> +ARMAsmParser::ApplyPrefixToExpr(const MCExpr *E,
>> + MCSymbolRefExpr::VariantKind Variant) {
>> + // Recurse over the given expression, rebuilding it to apply the given variant
>> + // to the leftmost symbol.
>> + if (Variant == MCSymbolRefExpr::VK_None)
>> + return E;
>> +
>> + switch (E->getKind()) {
>> + case MCExpr::Target:
>> + llvm_unreachable("Can't handle target expr yet");
>> + case MCExpr::Constant:
>> + llvm_unreachable("Can't handle lower16/upper16 of constant yet");
>> +
>> + case MCExpr::SymbolRef: {
>> + const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(E);
>> +
>> + if (SRE->getKind() != MCSymbolRefExpr::VK_None)
>> + return 0;
>> +
>> + return MCSymbolRefExpr::Create(&SRE->getSymbol(), Variant, getContext());
>> + }
>> +
>> + case MCExpr::Unary:
>> + llvm_unreachable("Can't handle unary expressions yet");
>> +
>> + case MCExpr::Binary: {
>> + const MCBinaryExpr *BE = cast<MCBinaryExpr>(E);
>> + const MCExpr *LHS = ApplyPrefixToExpr(BE->getLHS(), Variant);
>> + const MCExpr *RHS = BE->getRHS();
>> + if (!LHS)
>> + return 0;
>> +
>> + return MCBinaryExpr::Create(BE->getOpcode(), LHS, RHS, getContext());
>> + }
>> + }
>> +
>> + assert(0 && "Invalid expression kind!");
>> + return 0;
>> +}
>> +
>> /// \brief Given a mnemonic, split out possible predication code and carry
>> /// setting letters to form a canonical mnemonic and flags.
>> //
>>
>> Added: llvm/trunk/test/MC/ARM/elf-movt.s
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/elf-movt.s?rev=123292&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/MC/ARM/elf-movt.s (added)
>> +++ llvm/trunk/test/MC/ARM/elf-movt.s Tue Jan 11 17:53:41 2011
>> @@ -0,0 +1,14 @@
>> +@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi | FileCheck -check-prefix=ASM %s
>> + .syntax unified
>> + .text
>> + .globl barf
>> + .align 2
>> + .type barf,%function
>> +barf: @ @barf
>> +@ BB#0: @ %entry
>> + movw r0, :lower16:GOT-(.LPC0_2+8)
>> + movt r0, :upper16:GOT-(.LPC0_2+16)
>> +.LPC0_2:
>> +@ ASM: movw r0, :lower16:GOT-(.LPC0_2+8)
>> +@ ASM-NEXT: movt r0, :upper16:GOT-(.LPC0_2+16)
>> +
>>
>
> Ditto Evan's comments on the test case values.
LOL :-) Looks like I did a reply, not a reply-all to Evan's review
Reproduced here:
The 16 was there just to verify that the fixup on the text worked as
well. Yes, in real code, it would be +8 :-)
> Evan Wrote:
> I'd like to change :lower16: so it would become a MCExpr unary operator. However, that means the followings have different semantics:
>
> :lower16:GOT-(.LPC0_2+8)
> and
> :lower16:(GOT-(.LPC0_2+8))
>
> So your example should look like the second case, right?
Yes. That is correct.
FYI, There is some fragility in the AsmParser -
For example:
inputing this sequence
movw r0, :lower16:(GOT-(.LPC0_2+8))
movt r0, :upper16:GOT-.LPC0_2+8
produces this:
movw r0, :lower16:GOT-(.LPC0_2+8)
movt r0, (:upper16:GOT-.LPC0_2)+8
Ideally the :lower16: should be a lower precedence operator :-)
--
Thanks again for review, Jim and Evan! Much appreciated!
-jason
>
>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
More information about the llvm-commits
mailing list