<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 16 January 2014 14:27, Sasa Stankovic <span dir="ltr"><<a href="mailto:Sasa.Stankovic@rt-rk.com" target="_blank">Sasa.Stankovic@rt-rk.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Mark,<br>
<div class="im"><br>
> Can you post the earlier patch you had for adding a Mips MCExpr subclass?<br>
<br>
</div><div class=""><div class="h5">The patch is attached.</div></div></blockquote><div><br></div><div>Thanks. The basic approach here does look better.</div><div><br></div><div>Can you use Phabricator (<a href="http://llvm.org/docs/Phabricator.html">http://llvm.org/docs/Phabricator.html</a>) to send patches in future? It will make reviewing easier.</div>
<div><br></div><div><div>You need to add MipsMCExpr.cpp to CMakeLists.txt. More comments below...</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Index: test/MC/Mips/hilo-addressing.s<br>===================================================================<br>--- test/MC/Mips/hilo-addressing.s<span class="" style="white-space:pre"> </span>(revision 199425)<br>+++ test/MC/Mips/hilo-addressing.s<span class="" style="white-space:pre"> </span>(working copy)<br>
@@ -1,11 +1,39 @@<br>-# RUN: llvm-mc -show-encoding -triple mips-unknown-unknown %s | FileCheck %s<br>+# RUN: llvm-mc -show-encoding -triple mips-unknown-unknown %s \<br>+# RUN: | FileCheck %s -check-prefix=CHECK-ENC<br>
<br>- .ent hilo_test<br>- .equ addr, 0xdeadbeef<br>-# CHECK: # encoding: [0x3c,0x04,0xde,0xae]<br>- lui $4,%hi(addr)<br>-# CHECK: # encoding: [0x03,0xe0,0x00,0x08]<br>- jr $31<br>-# CHECK: # encoding: [0x80,0x82,0xbe,0xef]<br>
- lb $2,%lo(addr)($4)<br>- .end hilo_test<br>+# RUN: llvm-mc -filetype=obj -triple=mipsel-unknown-linux %s -o - \<br>+# RUN: | llvm-objdump -disassemble - | FileCheck %s -check-prefix=CHECK-INSTR<br>+<br>+# RUN: llvm-mc %s -filetype=obj -triple=mipsel-unknown-linux \<br>
+# RUN: | llvm-readobj -r | FileCheck %s -check-prefix=CHECK-REL<br>+<br>+<br>+# Check that 1 is added to the high 16 bits if bit 15 of the low part is 1.<br>+<br>+ .equ addr, 0xdeadbeef<br>+ lui $4, %hi(addr)<br>
+ lb $2, %lo(addr)($4)<br>+# CHECK-ENC: # encoding: [0x3c,0x04,0xde,0xae]<br>+# CHECK-ENC: # encoding: [0x80,0x82,0xbe,0xef]<br>+<br>+<br>+# Check that assembler can handle %hi(label1 - label2) and %lo(label1 - label2)<br>
+# expressions.<br>+<br>+$L1:<br>+ lui $4, %hi($L1-$L2)<br>+ addiu $4, $4, %lo($L1-$L2)<br>+$L2:<br>+ lui $5, %hi($L2-$L1)<br>+ lw $5, %lo($L2-$L1)($5)<br>+<br>+# CHECK-INSTR: lui $4, 0<br>
+# CHECK-INSTR: addiu $4, $4, -8<br>+# CHECK-INSTR: lui $5, 0<br>+# CHECK-INSTR: lw $5, 8($5)<br>+<br>+<br>+# Check that relocation is not emitted for %hi(label1 - label2) and<br>+# %lo(label1 - label2) expressions.<br>
+<br>+# CHECK-REL-NOT: R_MIPS<br>Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp<br>===================================================================<br>--- lib/Target/Mips/AsmParser/MipsAsmParser.cpp<span class="" style="white-space:pre"> </span>(revision 199425)<br>
+++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp<span class="" style="white-space:pre"> </span>(working copy)<br>@@ -8,6 +8,7 @@<br> //===----------------------------------------------------------------------===//<br> <br>
#include "MCTargetDesc/MipsMCTargetDesc.h"<br>+#include "MCTargetDesc/MipsMCExpr.h"<br> #include "MipsRegisterInfo.h"<br> #include "MipsTargetStreamer.h"<br> #include "llvm/ADT/APInt.h"<br>
@@ -1314,6 +1315,34 @@<br> if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {<br> const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr);<br> const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr);<br>
+<br>+ // Create target expression for %hi(sym1-sym2) and %lo(sym1-sym2)<br>+ // expressions.<br>+ if (BE->getOpcode() == MCBinaryExpr::Sub) {<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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?<br>
</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(LExp);<br>
+ const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(RExp);<br>+ MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);<br>+ if (LSRE && RSRE && (VK == MCSymbolRefExpr::VK_Mips_ABS_HI<br>
+ || VK == MCSymbolRefExpr::VK_Mips_ABS_LO)) {<br>+<br>+ // Currently, when parser parses %hi(sym1 - sym2), it will attach %hi<br>+ // relocation to both sym1 and sym2. Creating LSRE2 and RSRE2 is a<br>
+ // workaround that strips relocation from sym1 and sym2. This is<br>+ // needed for MipsMCExpr::EvaluateAsRelocatableImpl to work correctly.<br>+ const MCSymbolRefExpr *LSRE2<br>+ = MCSymbolRefExpr::Create(&LSRE->getSymbol(), getContext());<br>
+ const MCSymbolRefExpr *RSRE2<br>+ = MCSymbolRefExpr::Create(&RSRE->getSymbol(), getContext());<br>+<br>+ Res = MCBinaryExpr::Create(BE->getOpcode(), LSRE2, RSRE2, getContext());<br></blockquote>
<div><br></div><div>Along similar lines, you don't need to recreate an MCBinaryExpr, you can just reuse BE here.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ if (VK == MCSymbolRefExpr::VK_Mips_ABS_HI)<br>+ Res = MipsMCExpr::CreateHi(Res, getContext());<br>+ else<br>+ Res = MipsMCExpr::CreateLo(Res, getContext());<br>+ return Res;<br>+ }<br>
+ }<br>+<br> Res = MCBinaryExpr::Create(BE->getOpcode(), LExp, RExp, getContext());<br> return Res;<br> }<br>@@ -1342,6 +1371,8 @@<br> }<br> case MCExpr::Unary:<br> return isEvaluated(cast<MCUnaryExpr>(Expr)->getSubExpr());<br>
+ case MCExpr::Target:<br>+ return true;<br> default:<br> return false;<br> }<br>Index: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<br>===================================================================<br>
--- lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<span class="" style="white-space:pre"> </span>(revision 199425)<br>+++ lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<span class="" style="white-space:pre"> </span>(working copy)<br>
@@ -15,6 +15,7 @@<br> #include "MCTargetDesc/MipsBaseInfo.h"<br> #include "MCTargetDesc/MipsFixupKinds.h"<br> #include "MCTargetDesc/MipsMCTargetDesc.h"<br>+#include "MCTargetDesc/MipsMCExpr.h"<br>
#include "llvm/ADT/APFloat.h"<br> #include "llvm/ADT/Statistic.h"<br> #include "llvm/MC/MCCodeEmitter.h"<br>@@ -365,6 +366,28 @@<br> Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups);<br>
return Res;<br> }<br>+<br>+ if (Kind == MCExpr::Target) {<br>+ const MipsMCExpr *MipsExpr = cast<MipsMCExpr>(Expr);<br>+<br>+ MCFixupKind FixupKind;<br>+ switch (MipsExpr->getKind()) {<br>+ default: llvm_unreachable("Unsupported fixup");<br>
+ case MipsMCExpr::VK_Mips_ABS_HI:<br>+ FixupKind = MCFixupKind(IsMicroMips<br>+ ? Mips::fixup_MICROMIPS_HI16<br>+ : Mips::fixup_Mips_HI16);<br>+ break;<br>
+ case MipsMCExpr::VK_Mips_ABS_LO:<br>+ FixupKind = MCFixupKind(IsMicroMips<br>+ ? Mips::fixup_MICROMIPS_LO16<br>+ : Mips::fixup_Mips_LO16);<br>+ break;<br>
+ }<br>+ Fixups.push_back(MCFixup::Create(0, MipsExpr, FixupKind));<br>+ return 0;<br>+ }<br>+<br></blockquote><div><br></div><div>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?</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> if (Kind == MCExpr::SymbolRef) {<br>
Mips::Fixups FixupKind = Mips::Fixups(0);<br> <br>Index: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp<br>===================================================================<br>--- lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp<span class="" style="white-space:pre"> </span>(revision 0)<br>
+++ lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp<span class="" style="white-space:pre"> </span>(revision 0)<br>@@ -0,0 +1,117 @@<br>+//===-- MipsMCExpr.cpp - Mips specific MC expression classes --------------===//<br>+//<br>
+// The LLVM Compiler Infrastructure<br>+//<br>+// This file is distributed under the University of Illinois Open Source<br>+// License. See LICENSE.TXT for details.<br>+//<br>+//===----------------------------------------------------------------------===//<br>
+<br>+#define DEBUG_TYPE "mipsmcexpr"<br>+#include "MipsMCExpr.h"<br>+#include "llvm/MC/MCAssembler.h"<br>+#include "llvm/MC/MCContext.h"<br>+#include "llvm/MC/MCAsmInfo.h"<br>
+<br>+using namespace llvm;<br>+<br>+const MipsMCExpr*<br>+MipsMCExpr::Create(VariantKind Kind, const MCExpr *Expr,<br>+ MCContext &Ctx) {<br>+ return new (Ctx) MipsMCExpr(Kind, Expr);<br>+}<br>+<br>
+void MipsMCExpr::PrintImpl(raw_ostream &OS) const {<br>+ switch (Kind) {<br>+ default: llvm_unreachable("Invalid kind!");<br>+ case VK_Mips_ABS_LO: OS << "%lo"; break;<br>+ case VK_Mips_ABS_HI: OS << "%hi"; break;<br>
+ }<br>+<br>+ const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr);<br>+ const MCSymbolRefExpr *SRE1 = dyn_cast<MCSymbolRefExpr>(BE->getLHS());<br>+ const MCSymbolRefExpr *SRE2 = dyn_cast<MCSymbolRefExpr>(BE->getRHS());<br>
+ assert(SRE1 && SRE2<br>+ && "Only sym1-sym2 target expressions are currently supported.");<br>+<br>+ OS << '(';<br>+ OS << SRE1->getSymbol();<br>+ OS << '-';<br>
+ OS << SRE2->getSymbol();<br>+ OS << ')';<br></blockquote><div><br></div><div>Why not just call Expr->print() here?</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+}<br>+<br>+bool<br>+MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,<br>+ const MCAsmLayout *Layout) const {<br>+ MCValue Value;<br>+<br>+ if (!Layout || !getSubExpr()->EvaluateAsRelocatable(Value, *Layout))<br>
+ return false;<br>+<br>+ if (Value.isAbsolute()) {<br>+ int64_t Result = Value.getConstant();<br>+ switch (Kind) {<br>+ default:<br>+ llvm_unreachable("Invalid kind!");<br>+ case VK_Mips_ABS_LO:<br>
+ Result = Result & 0xffff;<br>+ break;<br>+ case VK_Mips_ABS_HI:<br>+ Result = ((Result + 0x8000) >> 16) & 0xffff;<br>+ break;<br>+ }<br>+ Res = MCValue::get(Result);<br>
+ } else {<br>+ MCContext &Context = Layout->getAssembler().getContext();<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ const MCSymbolRefExpr *Sym = Value.getSymA();<br>
+ MCSymbolRefExpr::VariantKind Modifier = Sym->getKind();<br>+ if (Modifier != MCSymbolRefExpr::VK_None)<br>+ return false;<br>+ switch (Kind) {<br>+ default:<br>+ llvm_unreachable("Invalid kind!");<br>
+ case VK_Mips_ABS_LO:<br>+ Modifier = MCSymbolRefExpr::VK_Mips_ABS_LO;<br>+ break;<br>+ case VK_Mips_ABS_HI:<br>+ Modifier = MCSymbolRefExpr::VK_Mips_ABS_HI;<br>+ break;<br>+ }<br>
+ Sym = MCSymbolRefExpr::Create(&Sym->getSymbol(), Modifier, Context);<br>+ Res = MCValue::get(Sym, Value.getSymB(), Value.getConstant());<br>+ }<br>+<br>+ return true;<br>+}<br>+<br>+// FIXME: This basically copies MCObjectStreamer::AddValueSymbols. Perhaps<br>
+// that method should be made public?<br>+static void AddValueSymbolsImpl(const MCExpr *Value, MCAssembler *Asm) {<br>+ switch (Value->getKind()) {<br>+ case MCExpr::Target:<br>+ llvm_unreachable("Can't handle nested target expr!");<br>
+<br>+ case MCExpr::Constant:<br>+ break;<br>+<br>+ case MCExpr::Binary: {<br>+ const MCBinaryExpr *BE = cast<MCBinaryExpr>(Value);<br>+ AddValueSymbolsImpl(BE->getLHS(), Asm);<br>+ AddValueSymbolsImpl(BE->getRHS(), Asm);<br>
+ break;<br>+ }<br>+<br>+ case MCExpr::SymbolRef:<br>+ Asm->getOrCreateSymbolData(cast<MCSymbolRefExpr>(Value)->getSymbol());<br>+ break;<br>+<br>+ case MCExpr::Unary:<br>+ AddValueSymbolsImpl(cast<MCUnaryExpr>(Value)->getSubExpr(), Asm);<br>
+ break;<br>+ }<br>+}<br>+<br>+void MipsMCExpr::AddValueSymbols(MCAssembler *Asm) const {<br>+ AddValueSymbolsImpl(getSubExpr(), Asm);<br>+}<br>Index: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h<br>===================================================================<br>
--- lib/Target/Mips/MCTargetDesc/MipsMCExpr.h<span class="" style="white-space:pre"> </span>(revision 0)<br>+++ lib/Target/Mips/MCTargetDesc/MipsMCExpr.h<span class="" style="white-space:pre"> </span>(revision 0)<br>@@ -0,0 +1,70 @@<br>
+//===-- MipsMCExpr.h - Mips specific MC expression classes ------*- C++ -*-===//<br>+//<br>+// The LLVM Compiler Infrastructure<br>+//<br>+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>+//<br>+//===----------------------------------------------------------------------===//<br>+<br>+#ifndef MIPSMCEXPR_H<br>+#define MIPSMCEXPR_H<br>+<br>+#include "llvm/MC/MCExpr.h"<br>
+#include "llvm/MC/MCValue.h"<br>+#include "llvm/MC/MCAsmLayout.h"<br>+<br>+namespace llvm {<br>+<br>+class MipsMCExpr : public MCTargetExpr {<br>+public:<br>+ enum VariantKind {<br>+ VK_Mips_None,<br>
+ VK_Mips_ABS_LO,<br>+ VK_Mips_ABS_HI<br>+ };<br>+<br>+private:<br>+ const VariantKind Kind;<br>+ const MCExpr *Expr;<br>+<br>+ explicit MipsMCExpr(VariantKind _Kind, const MCExpr *_Expr)<br>+ : Kind(_Kind), Expr(_Expr) {}<br>
+<br>+public:<br>+ static const MipsMCExpr *Create(VariantKind Kind, const MCExpr *Expr,<br>+ MCContext &Ctx);<br>+<br>+ static const MipsMCExpr *CreateLo(const MCExpr *Expr, MCContext &Ctx) {<br>
+ return Create(VK_Mips_ABS_LO, Expr, Ctx);<br>+ }<br>+<br>+ static const MipsMCExpr *CreateHi(const MCExpr *Expr, MCContext &Ctx) {<br>+ return Create(VK_Mips_ABS_HI, Expr, Ctx);<br>+ }<br>+<br>+ /// getOpcode - Get the kind of this expression.<br>
+ VariantKind getKind() const { return Kind; }<br>+<br>+ /// getSubExpr - Get the child of this expression.<br>+ const MCExpr *getSubExpr() const { return Expr; }<br>+<br>+ void PrintImpl(raw_ostream &OS) const;<br>
+ bool EvaluateAsRelocatableImpl(MCValue &Res,<br>+ const MCAsmLayout *Layout) const;<br>+ void AddValueSymbols(MCAssembler *) const;<br>+ const MCSection *FindAssociatedSection() const {<br>
+ return getSubExpr()->FindAssociatedSection();<br>+ }<br>+<br>+ // There are no TLS MipsMCExprs at the moment.<br>+ void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const {}<br>+<br>+ static bool classof(const MCExpr *E) {<br>
+ return E->getKind() == MCExpr::Target;<br>+ }<br>+};<br>+} // end namespace llvm<br>+<br>+#endif<br>+<br>Index: lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp<br>===================================================================<br>
--- lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp<span class="" style="white-space:pre"> </span>(revision 199425)<br>+++ lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp<span class="" style="white-space:pre"> </span>(working copy)<br>
@@ -14,6 +14,7 @@<br> #define DEBUG_TYPE "asm-printer"<br> #include "MipsInstPrinter.h"<br> #include "MipsInstrInfo.h"<br>+#include "MCTargetDesc/MipsMCExpr.h"<br> #include "llvm/ADT/StringExtras.h"<br>
#include "llvm/MC/MCExpr.h"<br> #include "llvm/MC/MCInst.h"<br>@@ -129,8 +130,10 @@<br> const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(BE->getRHS());<br> assert(SRE && CE && "Binary expression must be sym+const.");<br>
Offset = CE->getValue();<br>- }<br>- else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))<br>+ } else if (const MipsMCExpr *ME = dyn_cast<MipsMCExpr>(Expr)) {<br>+ ME->PrintImpl(OS);<br>+ return;<br>
+ } else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))<br> assert(false && "Unexpected MCExpr type.");<br></blockquote><div><br></div><div>Why not just add a general-purpose "else" which calls ME->print(OS)?</div>
<div><br></div><div>Cheers,</div><div>Mark</div><div><br></div></div></div></div>