Patch that implements %hi(sym1 - sym2) and %lo(sym1 - sym2) expressions for MIPS.

[sstankovic]

Summary

Patch implements %hi(sym1 - sym2) and %lo(sym1 - sym2) expressions for MIPS by creating target expression class MipsMCExpr.

[mseaborn]

Thanks for uploading this via Phabricator. Sorry for being slow to review this again this time around -- I wanted to take the time to try the patch locally and try modifying it.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

11 ↗

(On Diff #6573)

Nit: make sure the #include list stays sorted

1319 ↗

(On Diff #6573)

In your previous e-mail, you wrote:

If evaluateRelocExpr() is removed, then following instruction won't assemble: ...

I see what you mean. However, what I mean is that evaluateRelocExpr() shouldn't recurse when creating a MipsMCExpr.

So it would be cleaner to change these two lines to:

const MCSymbolRefExpr *LSRE = dyn_cast<MCSymbolRefExpr>(BE->getLHS());
const MCSymbolRefExpr *RSRE = dyn_cast<MCSymbolRefExpr>(BE->getRHS());

With that, the tests still pass.

1331 ↗

(On Diff #6573)

With the change above, you don't need to recreate the two MCSymbolRefExprs here. You can just pass LSRE and RSRE to MCBinaryExpr::Create().

1344 ↗

(On Diff #6573)

So then, for clarity, these two lines:

const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr);
const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr);

could be moved down to just before this line.

1374 ↗

(On Diff #6573)

This 'default' case can be removed now, after adding the MCExpr::Target case, otherwise it produces the warning "default label in switch which covers all enumeration values [-Wcovered-switch-default]".

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp

377 ↗

(On Diff #6573)

Note that this needs to be updated to isMicroMips(STI) on trunk now.

lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp

35 ↗

(On Diff #6573)

This check shouldn't be needed because this function can handle any subexpression.

lib/Target/Mips/MCTargetDesc/MipsMCExpr.h

31 ↗

(On Diff #6573)

Nit: you don't need a "_" prefix. You can write "Kind(Kind)" below.

[sstankovic]

Patch is updated.

[sstankovic]

Fixed the comment.

[mseaborn]

Thanks for addressing my comments. Just one more thing, about EvaluateAsRelocatableImpl(). I didn't understand that part when I reviewed the change on Wednesday, so I didn't comment on that part then, but having investigated I understand it now.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1318 ↗

(On Diff #6788)

Nit: With the change below, you could simplify this a little by using isa<> rather than dyn_cast<>.

1324 ↗

(On Diff #6788)

Minor issue: You can simplify this further: you don't need to recreate the same MCBinaryExpr. CreateHi() and CreateLo() can be passed Expr rather than Res.

lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp

37 ↗

(On Diff #6788)

In your earlier version of the patch, you had EvaluateAsRelocatableImpl() doing the shifting/masking for hi16. I didn't understand how it worked to remove this part, until I investigated further.

I had a look at the other *MCExpr.{h,cpp} files, and it seems there are there are two approaches to this:

1. Have EvaluateAsRelocatableImpl() do the shifting/masking. There's only one architecture that does this, PowerPC (see lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.cpp).

This approach would work for MIPS if you:

• change MipsMCCodeEmitter.cpp's MCExpr::Target handling to push a FK_Data_2 fixup instead of LO16/HI16 fixup;
• change adjustFixupValue() in MipsAsmBackend.cpp to accept FK_Data_2 in addition to FK_Data_4.

(This works -- I tried it.)

2. Do the shifting/masking elsewhere, leaving a trivial implementation of EvaluateAsRelocatableImpl().

For two architectures, EvaluateAsRelocatableImpl() just does "return false":
lib/Target/ARM/MCTargetDesc/ARMMCExpr.cpp
lib/Target/NVPTX/NVPTXMCExpr.h

For another two, EvaluateAsRelocatableImpl() delegates trivially to the subexpression:
lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp
lib/Target/Sparc/MCTargetDesc/SparcMCExpr.cpp
i.e.

AArch64MCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
                                         const MCAsmLayout *Layout) const {
  return getSubExpr()->EvaluateAsRelocatable(Res, *Layout);
}

Using approach 2 seems preferable since it's the most common in the codebase. It's what you're doing, except that your EvaluateAsRelocatableImpl() is a lot more complex and is closer to the PPC version.

Based on the AArch64/Sparc versions, you can simplify this to:

MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
                                      const MCAsmLayout *Layout) const {
  if (!Layout)
    return false;
  return getSubExpr()->EvaluateAsRelocatable(Res, *Layout);
}

So can you change it to that, please?

(The extra Layout check is necessary because getExprOpValue()'s call to EvaluateAsAbsolute() in MipsMCCodeEmitter.cpp causes this to be called with Layout==NULL.)

[sstankovic]

Patch is updated.

[mseaborn]

LGTM. When you commit this, you can put "Differential Revision: http://llvm-reviews.chandlerc.com/D2592" in the commit message to link back to the review.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1325 ↗

(On Diff #6825)

Nit: you could do "return MipsMCExpr::CreateHi(...)" here

test/MC/Mips/hilo-addressing.s

7 ↗

(On Diff #6825)

Can you make this llvm-mc invocation consistent with the previous one? i.e. "%s" at end, and consistently use or omit "-o -".

[sstankovic]

Patch is updated.

[petarj]

Closed by commit rL200783 (authored by @petarj).

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

 //===-- MipsAsmParser.cpp - Parse Mips assembly to MCInst instructions ----===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "MCTargetDesc/MipsMCExpr.h"
 #include "MCTargetDesc/MipsMCTargetDesc.h"
 #include "MipsRegisterInfo.h"
 #include "MipsTargetStreamer.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
if (const MCSymbolRefExpr *MSRE = dyn_cast<MCSymbolRefExpr>(Expr)) {
     // It's a symbol, create a symbolic expression from the symbol.
     StringRef Symbol = MSRE->getSymbol().getName();
     MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);
     Res = MCSymbolRefExpr::Create(Symbol, VK, getContext());
     return Res;
   }
 
   if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
+    MCSymbolRefExpr::VariantKind VK = getVariantKind(RelocStr);
+
+    // Check for %hi(sym1-sym2) and %lo(sym1-sym2) expressions.
+    if (isa<MCSymbolRefExpr>(BE->getLHS()) && isa<MCSymbolRefExpr>(BE->getRHS())
+        && (VK == MCSymbolRefExpr::VK_Mips_ABS_HI
+            || VK == MCSymbolRefExpr::VK_Mips_ABS_LO)) {
+      // Create target expression for %hi(sym1-sym2) and %lo(sym1-sym2).
+      if (VK == MCSymbolRefExpr::VK_Mips_ABS_HI)
+        return MipsMCExpr::CreateHi(Expr, getContext());
+      return MipsMCExpr::CreateLo(Expr, getContext());
+    }
+
     const MCExpr *LExp = evaluateRelocExpr(BE->getLHS(), RelocStr);
     const MCExpr *RExp = evaluateRelocExpr(BE->getRHS(), RelocStr);
     Res = MCBinaryExpr::Create(BE->getOpcode(), LExp, RExp, getContext());
     return Res;
   }
 
   if (const MCUnaryExpr *UN = dyn_cast<MCUnaryExpr>(Expr)) {
     const MCExpr *UnExp = evaluateRelocExpr(UN->getSubExpr(), RelocStr);
bool MipsAsmParser::isEvaluated(const MCExpr *Expr) {
   case MCExpr::Binary:
     if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
       if (!isEvaluated(BE->getLHS()))
         return false;
       return isEvaluated(BE->getRHS());
     }
   case MCExpr::Unary:
     return isEvaluated(cast<MCUnaryExpr>(Expr)->getSubExpr());
-  default:
-    return false;
+  case MCExpr::Target:
+    return true;
   }
   return false;
 }
 
 bool MipsAsmParser::parseRelocOperand(const MCExpr *&Res) {
   Parser.Lex();                          // Eat the % token.
   const AsmToken &Tok = Parser.getTok(); // Get next token, operation.
   if (Tok.isNot(AsmToken::Identifier))

llvm/trunk/lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp

 //===-- MipsInstPrinter.cpp - Convert Mips MCInst to assembly syntax ------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This class prints an Mips MCInst to a .s file.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
static void printExpr(const MCExpr *Expr, raw_ostream &OS) {
   int Offset = 0;
   const MCSymbolRefExpr *SRE;
 
   if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
     SRE = dyn_cast<MCSymbolRefExpr>(BE->getLHS());
     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->print(OS);
+    return;
+  } else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
     assert(false && "Unexpected MCExpr type.");
 
   MCSymbolRefExpr::VariantKind Kind = SRE->getKind();
 
   switch (Kind) {
   default:                                 llvm_unreachable("Invalid kind!");
   case MCSymbolRefExpr::VK_None:           break;
   case MCSymbolRefExpr::VK_Mips_GPREL:     OS << "%gp_rel("; break;

llvm/trunk/lib/Target/Mips/MCTargetDesc/CMakeLists.txt

 add_llvm_library(LLVMMipsDesc
   MipsAsmBackend.cpp
   MipsMCAsmInfo.cpp
   MipsMCCodeEmitter.cpp
+  MipsMCExpr.cpp
   MipsMCTargetDesc.cpp
   MipsELFObjectWriter.cpp
   MipsTargetStreamer.cpp
   )

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp

 //===-- MipsMCCodeEmitter.cpp - Convert Mips Code to Machine Code ---------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the MipsMCCodeEmitter class.
 //
 //===----------------------------------------------------------------------===//
 //
 #define DEBUG_TYPE "mccodeemitter"
 #include "MCTargetDesc/MipsBaseInfo.h"
 #include "MCTargetDesc/MipsFixupKinds.h"
+#include "MCTargetDesc/MipsMCExpr.h"
 #include "MCTargetDesc/MipsMCTargetDesc.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
if (Kind == MCExpr::Constant) {
     return cast<MCConstantExpr>(Expr)->getValue();
   }
 
   if (Kind == MCExpr::Binary) {
     unsigned Res = getExprOpValue(cast<MCBinaryExpr>(Expr)->getLHS(), Fixups, STI);
     Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups, STI);
     return Res;
   }
+
+  if (Kind == MCExpr::Target) {
+    const MipsMCExpr *MipsExpr = cast<MipsMCExpr>(Expr);
+
+    Mips::Fixups FixupKind = Mips::Fixups(0);
+    switch (MipsExpr->getKind()) {
+    default: llvm_unreachable("Unsupported fixup kind for target expression!");
+    case MipsMCExpr::VK_Mips_ABS_HI:
+      FixupKind = isMicroMips(STI) ? Mips::fixup_MICROMIPS_HI16
+                                   : Mips::fixup_Mips_HI16;
+      break;
+    case MipsMCExpr::VK_Mips_ABS_LO:
+      FixupKind = isMicroMips(STI) ? Mips::fixup_MICROMIPS_LO16
+                                   : Mips::fixup_Mips_LO16;
+      break;
+    }
+    Fixups.push_back(MCFixup::Create(0, MipsExpr, MCFixupKind(FixupKind)));
+    return 0;
+  }
+
   if (Kind == MCExpr::SymbolRef) {
     Mips::Fixups FixupKind = Mips::Fixups(0);
 
     switch(cast<MCSymbolRefExpr>(Expr)->getKind()) {
     default: llvm_unreachable("Unknown fixup kind!");
       break;
     case MCSymbolRefExpr::VK_Mips_GPOFF_HI :
       FixupKind = Mips::fixup_Mips_GPOFF_HI;

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsMCExpr.h

+//===-- 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

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp

+//===-- 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;
+  }
+
+  OS << '(';
+  Expr->print(OS);
+  OS << ')';
+}
+
+bool
+MipsMCExpr::EvaluateAsRelocatableImpl(MCValue &Res,
+                                      const MCAsmLayout *Layout) const {
+  if (!Layout)
+    return false;
+  return getSubExpr()->EvaluateAsRelocatable(Res, *Layout);
+}
+
+// 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);
+}

llvm/trunk/test/MC/Mips/hilo-addressing.s

-# 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
+
+# RUN: llvm-mc -filetype=obj -triple=mipsel-unknown-linux %s \
+# RUN:  | llvm-objdump -disassemble - | FileCheck %s -check-prefix=CHECK-INSTR
+
+# RUN: llvm-mc -filetype=obj -triple=mipsel-unknown-linux %s \
+# 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.
 
-  .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
+        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:
+        # Emit zeros so that difference between $L3 and $L1 is 0x30124 bytes.
+        .fill 0x30124-8
+$L2:
+        lui     $4, %hi($L3-$L1)
+        addiu   $4, $4, %lo($L3-$L1)
+# CHECK-INSTR:    lui     $4, 3
+# CHECK-INSTR:    addiu   $4, $4, 292
+
+$L3:
+        lui     $5, %hi($L2-$L3)
+        lw      $5, %lo($L2-$L3)($5)
+# 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