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

Mark Seaborn mseaborn at chromium.org
Wed Apr 2 08:23:22 PDT 2014


  > (The file tests that R_MIPS_HIGHER and R_MIPS_HIGHEST
  > relocations are emitted in the .o file. Since it uses
  > -force-mips-long-branch option, it was created when
  > MipsLongBranch's implementation was emitting R_MIPS_HIGHER and
  > R_MIPS_HIGHEST relocations in the .o file, and was disabled
  > when MipsLongBranch started to directly calculate offsets.)

  Can you put that explanation in the commit message please?  And also say in the commit message that you're removing "ABS_" for consistency between enums.

  LGTM.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1452
@@ +1451,3 @@
+    // It's a constant, evaluate reloc value.
+    short Val;
+    switch (getVariantKind(RelocStr)) {
----------------
uint16_t would be preferred here, I think.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1472
@@ -1466,1 +1471,3 @@
+    default:
+      llvm_unreachable("Unsupported reloc value!");
     }
----------------
AFAICT, it's not unreachable, because assembly could contain %gottprel(CONST).  So this should be report_fatal_error() instead.  llvm_unreachable gives undefined behaviour in release builds.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.cpp:30
@@ +29,3 @@
+
+  // We support expressions of the form "sym1 binop1 sym2 binop2 const",
+  // where "binop2 const" is optional.
----------------
Maybe write "(sym1 binop1 sym2) binop2 const" since you're checking for a particular grouping below.

================
Comment at: test/MC/Mips/higher-highest-addressing.s:17
@@ +16,3 @@
+# CHECK-REL: Relocations [
+# CHECK-REL:     0x{{[0-9,A-F]+}} R_MIPS_HIGHEST
+# CHECK-REL:     0x{{[0-9,A-F]+}} R_MIPS_HIGHER
----------------
Use CHECK-REL-NEXT?

================
Comment at: test/MC/Mips/higher-highest-addressing.s:59
@@ +58,2 @@
+
+# CHECK-REL-NOT:    R_MIPS
----------------
This would be checked by using CHECK-REL-NEXT above, so you could drop this.

================
Comment at: test/MC/Mips/higher-highest.s:17
@@ +16,3 @@
+# to %higher.
+$L1:
+        lui     $6,    %highest($L2-$L1+0x300047FFF7FF7)
----------------
Mark Seaborn wrote:
> Maybe remove these labels if they're not needed for the test?
Oops, sorry, I didn't notice that these labels are used in the test.


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



More information about the llvm-commits mailing list