[PATCH] Extend MipsMCExpr class to handle %higher(sym1 - sym2 + const) and %highest(sym1 - sym2 + const) relocations.

Mark Seaborn mseaborn at chromium.org
Mon Mar 31 13:06:36 PDT 2014



================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h:40
@@ -35,2 +39,3 @@
+
   static const MipsMCExpr *Create(VariantKind Kind, const MCExpr *Expr,
                                   MCContext &Ctx);
----------------
This isn't used any more, is it?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h:25
@@ +24,3 @@
+    VK_Mips_ABS_HI,
+    VK_Mips_HIGHER,
+    VK_Mips_HIGHEST
----------------
Why do LO/HI have "ABS_" in the name while HIGHER/HIGHEST don't?

This looks like an inconsistency copied from include/llvm/MC/MCExpr.h and lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h.  Would it make sense to drop "ABS" from the names here?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp:35
@@ +34,3 @@
+      return false;
+    BE = dyn_cast<MCBinaryExpr>(BE->getLHS());
+  }
----------------
Would be better as cast<>.

================
Comment at: test/MC/Mips/higher-highest.s:3
@@ +2,3 @@
+# RUN:  | llvm-objdump -disassemble -triple mips64el - | FileCheck %s
+
+# RUN: llvm-mc -filetype=obj -triple=mips64el-unknown-linux -mcpu=mips64r2 %s \
----------------
BTW, there is already "test/MC/Mips/higher_highest.ll".  Adding a file with a very similar name is somewhat confusing.

Maybe call this "higher-highest-addressing.s" to match your "hilo-addressing.s"?

Since higher_highest.ll is currently disabled, and since it tests at the wrong level (.ll) for test/MC, would it make sense to remove it?

================
Comment at: test/MC/Mips/higher-highest.s:17
@@ +16,3 @@
+# to %higher.
+$L1:
+        lui     $6,    %highest($L2-$L1+0x300047FFF7FF7)
----------------
Maybe remove these labels if they're not needed for the test?


http://llvm-reviews.chandlerc.com/D3230



More information about the llvm-commits mailing list