[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