[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