%hi($tmp) and %lo($tmp) relocations for Mips backend, where $tmp = sym1 - sym2
Mark Seaborn
mseaborn at chromium.org
Thu Jan 16 18:07:55 PST 2014
On 16 January 2014 14:27, Sasa Stankovic <Sasa.Stankovic at rt-rk.com> wrote:
> Hi Mark,
>
> > Can you post the earlier patch you had for adding a Mips MCExpr subclass?
>
> The patch is attached.
>
Thanks. The basic approach here does look better.
Can you use Phabricator (http://llvm.org/docs/Phabricator.html) to send
patches in future? It will make reviewing easier.
You need to add MipsMCExpr.cpp to CMakeLists.txt. More comments below...
Index: test/MC/Mips/hilo-addressing.s
> ===================================================================
> --- test/MC/Mips/hilo-addressing.s (revision 199425)
> +++ test/MC/Mips/hilo-addressing.s (working copy)
> @@ -1,11 +1,39 @@
> -# RUN: llvm-mc -show-encoding -triple mips-unknown-unknown %s | FileCheck
> %s
> +# RUN: llvm-mc -show-encoding -triple mips-unknown-unknown %s \
> +# RUN: | FileCheck %s -check-prefix=CHECK-ENC
>
> - .ent hilo_test
> - .equ addr, 0xdeadbeef
> -# CHECK: # encoding: [0x3c,0x04,0xde,0xae]
> - lui $4,%hi(addr)
> -# CHECK: # encoding: [0x03,0xe0,0x00,0x08]
> - jr $31
> -# CHECK: # encoding: [0x80,0x82,0xbe,0xef]
> - lb $2,%lo(addr)($4)
> - .end hilo_test
> +# RUN: llvm-mc -filetype=obj -triple=mipsel-unknown-linux %s -o - \
> +# RUN: | llvm-objdump -disassemble - | FileCheck %s
> -check-prefix=CHECK-INSTR
> +
> +# RUN: llvm-mc %s -filetype=obj -triple=mipsel-unknown-linux \
> +# RUN: | llvm-readobj -r | FileCheck %s -check-prefix=CHECK-REL
> +
> +
> +# Check that 1 is added to the high 16 bits if bit 15 of the low part is
> 1.
> +
> + .equ addr, 0xdeadbeef
> + lui $4, %hi(addr)
> + lb $2, %lo(addr)($4)
> +# CHECK-ENC: # encoding: [0x3c,0x04,0xde,0xae]
> +# CHECK-ENC: # encoding: [0x80,0x82,0xbe,0xef]
> +
> +
> +# Check that assembler can handle %hi(label1 - label2) and %lo(label1 -
> label2)
> +# expressions.
> +
> +$L1:
> + lui $4, %hi($L1-$L2)
> + addiu $4, $4, %lo($L1-$L2)
> +$L2:
> + lui $5, %hi($L2-$L1)
> + lw $5, %lo($L2-$L1)($5)
> +
> +# CHECK-INSTR: lui $4, 0
> +# CHECK-INSTR: addiu $4, $4, -8
> +# CHECK-INSTR: lui $5, 0
> +# CHECK-INSTR: lw $5, 8($5)
> +
> +
> +# Check that relocation is not emitted for %hi(label1 - label2) and
> +# %lo(label1 - label2) expressions.
> +
> +# CHECK-REL-NOT: R_MIPS
> Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp
> ===================================================================
> --- lib/Target/Mips/AsmParser/MipsAsmParser.cpp (revision 199425)
> +++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp (working copy)
> @@ -8,6 +8,7 @@
>
> //===----------------------------------------------------------------------===//
>
> #include "MCTargetDesc/MipsMCTargetDesc.h"
> +#include "MCTargetDesc/MipsMCExpr.h"
> #include "MipsRegisterInfo.h"
> #include "MipsTargetStreamer.h"
> #include "llvm/ADT/APInt.h"
> @@ -1314,6 +1315,34 @@
> if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
> const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr);
> const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr);
> +
> + // Create target expression for %hi(sym1-sym2) and %lo(sym1-sym2)
> + // expressions.
> + if (BE->getOpcode() == MCBinaryExpr::Sub) {
>
Why restrict this to Sub? If I change this line to "if (true)", all of the
tests (test/MC/Mips and test/CodeGen/Mips) still pass.
This evaluateRelocExpr() function is what is doing the incorrect conversion
from %hi(X - Y) to (%hi(X) - %hi(Y)), so really those two recursive calls
to evaluateRelocExpr() above should go away. I don't think there's any
reason for evaluateRelocExpr() to recurse on an MCBinaryExpr, is there?
> + const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(LExp);
> + const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(RExp);
> + MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);
> + if (LSRE && RSRE && (VK == MCSymbolRefExpr::VK_Mips_ABS_HI
> + || VK == MCSymbolRefExpr::VK_Mips_ABS_LO)) {
> +
> + // Currently, when parser parses %hi(sym1 - sym2), it will attach
> %hi
> + // relocation to both sym1 and sym2. Creating LSRE2 and RSRE2 is
> a
> + // workaround that strips relocation from sym1 and sym2. This is
> + // needed for MipsMCExpr::EvaluateAsRelocatableImpl to work
> correctly.
> + const MCSymbolRefExpr *LSRE2
> + = MCSymbolRefExpr::Create(&LSRE->getSymbol(), getContext());
> + const MCSymbolRefExpr *RSRE2
> + = MCSymbolRefExpr::Create(&RSRE->getSymbol(), getContext());
> +
> + Res = MCBinaryExpr::Create(BE->getOpcode(), LSRE2, RSRE2,
> getContext());
>
Along similar lines, you don't need to recreate an MCBinaryExpr, you can
just reuse BE here.
> + if (VK == MCSymbolRefExpr::VK_Mips_ABS_HI)
> + Res = MipsMCExpr::CreateHi(Res, getContext());
> + else
> + Res = MipsMCExpr::CreateLo(Res, getContext());
> + return Res;
> + }
> + }
> +
> Res = MCBinaryExpr::Create(BE->getOpcode(), LExp, RExp, getContext());
> return Res;
> }
> @@ -1342,6 +1371,8 @@
> }
> case MCExpr::Unary:
> return isEvaluated(cast<MCUnaryExpr>(Expr)->getSubExpr());
> + case MCExpr::Target:
> + return true;
> default:
> return false;
> }
> Index: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp (revision 199425)
> +++ lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp (working copy)
> @@ -15,6 +15,7 @@
> #include "MCTargetDesc/MipsBaseInfo.h"
> #include "MCTargetDesc/MipsFixupKinds.h"
> #include "MCTargetDesc/MipsMCTargetDesc.h"
> +#include "MCTargetDesc/MipsMCExpr.h"
> #include "llvm/ADT/APFloat.h"
> #include "llvm/ADT/Statistic.h"
> #include "llvm/MC/MCCodeEmitter.h"
> @@ -365,6 +366,28 @@
> Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups);
> return Res;
> }
> +
> + if (Kind == MCExpr::Target) {
> + const MipsMCExpr *MipsExpr = cast<MipsMCExpr>(Expr);
> +
> + MCFixupKind FixupKind;
> + switch (MipsExpr->getKind()) {
> + default: llvm_unreachable("Unsupported fixup");
> + case MipsMCExpr::VK_Mips_ABS_HI:
> + FixupKind = MCFixupKind(IsMicroMips
> + ? Mips::fixup_MICROMIPS_HI16
> + : Mips::fixup_Mips_HI16);
> + break;
> + case MipsMCExpr::VK_Mips_ABS_LO:
> + FixupKind = MCFixupKind(IsMicroMips
> + ? Mips::fixup_MICROMIPS_LO16
> + : Mips::fixup_Mips_LO16);
> + break;
> + }
> + Fixups.push_back(MCFixup::Create(0, MipsExpr, FixupKind));
> + return 0;
> + }
> +
>
I'm not sure what case this covers, but if I remove it the tests pass, so
maybe either leave it out or add a test?
> if (Kind == MCExpr::SymbolRef) {
> Mips::Fixups FixupKind = Mips::Fixups(0);
>
> Index: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp (revision 0)
> +++ lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp (revision 0)
> @@ -0,0 +1,117 @@
> +//===-- MipsMCExpr.cpp - Mips specific MC expression classes
> --------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
>
> +//===----------------------------------------------------------------------===//
> +
> +#define DEBUG_TYPE "mipsmcexpr"
> +#include "MipsMCExpr.h"
> +#include "llvm/MC/MCAssembler.h"
> +#include "llvm/MC/MCContext.h"
> +#include "llvm/MC/MCAsmInfo.h"
> +
> +using namespace llvm;
> +
> +const MipsMCExpr*
> +MipsMCExpr::Create(VariantKind Kind, const MCExpr *Expr,
> + MCContext &Ctx) {
> + return new (Ctx) MipsMCExpr(Kind, Expr);
> +}
> +
> +void MipsMCExpr::PrintImpl(raw_ostream &OS) const {
> + switch (Kind) {
> + default: llvm_unreachable("Invalid kind!");
> + case VK_Mips_ABS_LO: OS << "%lo"; break;
> + case VK_Mips_ABS_HI: OS << "%hi"; break;
> + }
> +
> + const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr);
> + const MCSymbolRefExpr *SRE1 = dyn_cast<MCSymbolRefExpr>(BE->getLHS());
> + const MCSymbolRefExpr *SRE2 = dyn_cast<MCSymbolRefExpr>(BE->getRHS());
> + assert(SRE1 && SRE2
> + && "Only sym1-sym2 target expressions are currently supported.");
> +
> + OS << '(';
> + OS << SRE1->getSymbol();
> + OS << '-';
> + OS << SRE2->getSymbol();
> + OS << ')';
>
Why not just call Expr->print() here?
+}
> +
> +bool
> +MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
> + const MCAsmLayout *Layout) const {
> + MCValue Value;
> +
> + if (!Layout || !getSubExpr()->EvaluateAsRelocatable(Value, *Layout))
> + return false;
> +
> + if (Value.isAbsolute()) {
> + int64_t Result = Value.getConstant();
> + switch (Kind) {
> + default:
> + llvm_unreachable("Invalid kind!");
> + case VK_Mips_ABS_LO:
> + Result = Result & 0xffff;
> + break;
> + case VK_Mips_ABS_HI:
> + Result = ((Result + 0x8000) >> 16) & 0xffff;
> + break;
> + }
> + Res = MCValue::get(Result);
> + } else {
> + MCContext &Context = Layout->getAssembler().getContext();
>
It looks like you don't need the code for this else branch since you never
create an MipsMCExpr wrapping an MCSymbolRefExpr, so this can just return
false.
> + const MCSymbolRefExpr *Sym = Value.getSymA();
> + MCSymbolRefExpr::VariantKind Modifier = Sym->getKind();
> + if (Modifier != MCSymbolRefExpr::VK_None)
> + return false;
> + switch (Kind) {
> + default:
> + llvm_unreachable("Invalid kind!");
> + case VK_Mips_ABS_LO:
> + Modifier = MCSymbolRefExpr::VK_Mips_ABS_LO;
> + break;
> + case VK_Mips_ABS_HI:
> + Modifier = MCSymbolRefExpr::VK_Mips_ABS_HI;
> + break;
> + }
> + Sym = MCSymbolRefExpr::Create(&Sym->getSymbol(), Modifier, Context);
> + Res = MCValue::get(Sym, Value.getSymB(), Value.getConstant());
> + }
> +
> + return true;
> +}
> +
> +// FIXME: This basically copies MCObjectStreamer::AddValueSymbols. Perhaps
> +// that method should be made public?
> +static void AddValueSymbolsImpl(const MCExpr *Value, MCAssembler *Asm) {
> + switch (Value->getKind()) {
> + case MCExpr::Target:
> + llvm_unreachable("Can't handle nested target expr!");
> +
> + case MCExpr::Constant:
> + break;
> +
> + case MCExpr::Binary: {
> + const MCBinaryExpr *BE = cast<MCBinaryExpr>(Value);
> + AddValueSymbolsImpl(BE->getLHS(), Asm);
> + AddValueSymbolsImpl(BE->getRHS(), Asm);
> + break;
> + }
> +
> + case MCExpr::SymbolRef:
> + Asm->getOrCreateSymbolData(cast<MCSymbolRefExpr>(Value)->getSymbol());
> + break;
> +
> + case MCExpr::Unary:
> + AddValueSymbolsImpl(cast<MCUnaryExpr>(Value)->getSubExpr(), Asm);
> + break;
> + }
> +}
> +
> +void MipsMCExpr::AddValueSymbols(MCAssembler *Asm) const {
> + AddValueSymbolsImpl(getSubExpr(), Asm);
> +}
> Index: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsMCExpr.h (revision 0)
> +++ lib/Target/Mips/MCTargetDesc/MipsMCExpr.h (revision 0)
> @@ -0,0 +1,70 @@
> +//===-- MipsMCExpr.h - Mips specific MC expression classes ------*- C++
> -*-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
>
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef MIPSMCEXPR_H
> +#define MIPSMCEXPR_H
> +
> +#include "llvm/MC/MCExpr.h"
> +#include "llvm/MC/MCValue.h"
> +#include "llvm/MC/MCAsmLayout.h"
> +
> +namespace llvm {
> +
> +class MipsMCExpr : public MCTargetExpr {
> +public:
> + enum VariantKind {
> + VK_Mips_None,
> + VK_Mips_ABS_LO,
> + VK_Mips_ABS_HI
> + };
> +
> +private:
> + const VariantKind Kind;
> + const MCExpr *Expr;
> +
> + explicit MipsMCExpr(VariantKind _Kind, const MCExpr *_Expr)
> + : Kind(_Kind), Expr(_Expr) {}
> +
> +public:
> + static const MipsMCExpr *Create(VariantKind Kind, const MCExpr *Expr,
> + MCContext &Ctx);
> +
> + static const MipsMCExpr *CreateLo(const MCExpr *Expr, MCContext &Ctx) {
> + return Create(VK_Mips_ABS_LO, Expr, Ctx);
> + }
> +
> + static const MipsMCExpr *CreateHi(const MCExpr *Expr, MCContext &Ctx) {
> + return Create(VK_Mips_ABS_HI, Expr, Ctx);
> + }
> +
> + /// getOpcode - Get the kind of this expression.
> + VariantKind getKind() const { return Kind; }
> +
> + /// getSubExpr - Get the child of this expression.
> + const MCExpr *getSubExpr() const { return Expr; }
> +
> + void PrintImpl(raw_ostream &OS) const;
> + bool EvaluateAsRelocatableImpl(MCValue &Res,
> + const MCAsmLayout *Layout) const;
> + void AddValueSymbols(MCAssembler *) const;
> + const MCSection *FindAssociatedSection() const {
> + return getSubExpr()->FindAssociatedSection();
> + }
> +
> + // There are no TLS MipsMCExprs at the moment.
> + void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const {}
> +
> + static bool classof(const MCExpr *E) {
> + return E->getKind() == MCExpr::Target;
> + }
> +};
> +} // end namespace llvm
> +
> +#endif
> +
> Index: lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp
> ===================================================================
> --- lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp (revision 199425)
> +++ lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp (working copy)
> @@ -14,6 +14,7 @@
> #define DEBUG_TYPE "asm-printer"
> #include "MipsInstPrinter.h"
> #include "MipsInstrInfo.h"
> +#include "MCTargetDesc/MipsMCExpr.h"
> #include "llvm/ADT/StringExtras.h"
> #include "llvm/MC/MCExpr.h"
> #include "llvm/MC/MCInst.h"
> @@ -129,8 +130,10 @@
> const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(BE->getRHS());
> assert(SRE && CE && "Binary expression must be sym+const.");
> Offset = CE->getValue();
> - }
> - else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
> + } else if (const MipsMCExpr *ME = dyn_cast<MipsMCExpr>(Expr)) {
> + ME->PrintImpl(OS);
> + return;
> + } else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
> assert(false && "Unexpected MCExpr type.");
>
Why not just add a general-purpose "else" which calls ME->print(OS)?
Cheers,
Mark
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140116/66a9d8af/attachment.html>
More information about the llvm-commits
mailing list