<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>