| Message ID | 0DA23CC379F5F945ACB41CF394B982776A69BD79@LEMAIL01.le.imgtec.org |
|---|---|
| State | New |
| Headers | show |
Hi Andrew, Thanks for working on this it is a painful area. There's a bit more to do but this is cleaning up some sneaky bugs. Can you create a GCC bugzilla entry if you haven't already as we should record where these bugs exist and when they are fixed? See my comments but I think that you are fixing more variants of this bug than your summary states so we need to capture the detail on what code is affected by these issues. Andrew Bennett <Andrew.Bennett@imgtec.com> writes: > different offsets. Lets show this with an example C program. > > struct > { > short s; > unsigned long long l; > } h; > > void foo (void) > { > h.l = 0; > } > > When this is compiled for MIPS it produces the following assembly: > > lui $2,%hi(h+8) > sw $0,%lo(h+8)($2) > jr $31 > sw $0,%lo(h+12)($2) This looks like a stale example h+8 implies 8 bytes of data preceding 'l'. >diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c >index 6cdda3b..f07e433 100644 >--- a/gcc/config/mips/mips.c >+++ b/gcc/config/mips/mips.c >@@ -2354,6 +2354,38 @@ mips_valid_lo_sum_p (enum mips_symbol_type symbol_type, machine_mode mode) > return true; > } > >+/* Return true if X in LO_SUM (REG, X) is a valid. */ is a valid... I would however merge this code into mips_valid_lo_sum_p as the new function name is fairly confusing. Both call sites for this function have the symbol_type available which mips_valid_lo_sum_p requires and also have the mode, reg, x so just add those to mips_valid_lo_sum_p. >+ >+bool >+mips_valid_lo_sum_lo_part_p (machine_mode mode, rtx reg, rtx x) >+{ >+ rtx symbol = NULL; three space indent. >+ >+ if (mips_abi != ABI_32) >+ return true; I don't think this is limited to o32. I was thinking this was just about splitting a multi-word unaligned access but actually the test cases in this patch show that it is also about accessing unaligned elements in a structure using the same 'hi' part for multiple 'lo' parts with differing offsets. In the end I think there is no word size or abi specific issues here; it is quite general. >+ if (mode == BLKmode) >+ return true; Why is this special? Does the core GCC code ensure that lo_sum on a BLKmode cannot have a constant offset greater than alignment? >+ if (reg && REG_P (reg) && REGNO (reg) == GLOBAL_POINTER_REGNUM) >+ return true; I don't think reg need be an optional argument it is available at both call sites. A comment to say why offsets from the global pointer are not affected would also be useful. >+ >+ if (GET_CODE (x) == CONST >+ && GET_CODE (XEXP (x, 0)) == PLUS >+ && GET_CODE (XEXP (XEXP (x, 0), 0)) == SYMBOL_REF) >+ symbol = XEXP (XEXP (x, 0), 0); >+ else if (GET_CODE (x) == SYMBOL_REF) >+ symbol = x; >+ >+ if (symbol >+ && SYMBOL_REF_DECL (symbol) >+ && (GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT) > >+ (DECL_ALIGN_UNIT (SYMBOL_REF_DECL (symbol)))) This needs another bracket to cover the multiline '>' condition. >+ return false; >+ >+ return true; >+} >+ > /* Return true if X is a valid address for machine mode MODE. If it is, > fill in INFO appropriately. STRICT_P is true if REG_OK_STRICT is in > effect. */ >@@ -2394,7 +2426,8 @@ mips_classify_address (struct mips_address_info *info, rtx x, > info->symbol_type > = mips_classify_symbolic_expression (info->offset, SYMBOL_CONTEXT_MEM); > return (mips_valid_base_register_p (info->reg, mode, strict_p) >- && mips_valid_lo_sum_p (info->symbol_type, mode)); >+ && mips_valid_lo_sum_p (info->symbol_type, mode) >+ && mips_valid_lo_sum_lo_part_p (mode, info->reg, info->offset)); As above this can become: && mips_valid_lo_sum_p (info->symbol_type, mode, info-reg, info->offset) > > case CONST_INT: > /* Small-integer addresses don't occur very often, but they >@@ -3143,6 +3176,8 @@ mips_split_symbol (rtx temp, rtx addr, machine_mode mode, rtx *low_out) > high = gen_rtx_HIGH (Pmode, copy_rtx (addr)); > high = mips_force_temporary (temp, high); > *low_out = gen_rtx_LO_SUM (Pmode, high, addr); >+ if (!mips_valid_lo_sum_lo_part_p (mode, NULL, addr)) tab indent. This can also become: if (!mips_valid_lo_sum_p (symbol_type, mode, high, addr)) >+ *low_out = mips_force_temporary (temp, *low_out); > break; > } > return true; General comment on the tests... I don't think any of the '-mfp??' related tests are actually doing anything with the FPU. All the load/store operations are happening in GP registers so the FP mode is irrelevant. This makes some tests redundant. >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c >new file mode 100644 >index 0000000..dc81fd9 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c >@@ -0,0 +1,14 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ Is the -mcode-readable=no so that these checks work for MIPS16? >+/* { dg-final { scan-assembler-not "%lo\\(h\\+6\\)" } } */ >+ >+struct __attribute__((packed)) >+{ >+ short s; >+ unsigned long long l; indent 2 space. >+} h; >+ >+void foo (void) void foo (void) throughout the tests. >+{ >+ h.l = 0; indent 2 space. >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c >new file mode 100644 >index 0000000..da80b67 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c >@@ -0,0 +1,19 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfpxx isa=2" } */ >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ >+/* { dg-final { scan-assembler "%lo\\(f\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ This should be checking that there is no %lo(a_d+4) anywhere in the code. >+ >+double d; >+double __attribute__((aligned(1))) a_d; >+float f; >+float __attribute__((aligned(1))) a_f; >+ >+void foo (void) >+{ >+ d = 0; >+ f = 0; >+ a_d = 0; >+ a_f = 0; indent 2 space. >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c >new file mode 100644 >index 0000000..b2679f9 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c >@@ -0,0 +1,17 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ This appears to be another form of the bug but is not mentioned in the summary. Current GCC generates the %lo that must not appear in this test. >+ >+struct __attribute__((packed)) >+{ >+ char c; >+ short s; >+ int i; indent 2 space >+} h; >+ >+void foo (void) >+{ >+ h.c = 0; >+ h.s = 0; >+ h.i = 0; indent 2 space >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c >new file mode 100644 >index 0000000..1b9a0b1 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c >@@ -0,0 +1,13 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ >+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ >+ >+unsigned long long l; >+unsigned long long __attribute__((aligned(1))) a_l; >+ >+void foo (void) >+{ >+ l = 0; >+ a_l = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c >new file mode 100644 >index 0000000..a01c1d8 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c >@@ -0,0 +1,15 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ >+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ Why not check 'd' and 'a_d' as well? >+ >+long long l; >+double d; >+ >+long long __attribute__((aligned(1))) a_l; >+double __attribute__((aligned(1))) a_d; >+ >+void foo (void) >+{ >+ d = (double) l; >+ a_d = (double) a_l; indent 2 space. >+} >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c >new file mode 100644 >index 0000000..496c078 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c >@@ -0,0 +1,16 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ >+ >+struct __attribute__((packed)) >+{ >+ short s; >+ double d; >+ float f; indent 2 space... throughout, I won't mark each one from here on. >+} h; >+ >+void foo (void) >+{ >+ h.d = 0; >+ h.f = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c >new file mode 100644 >index 0000000..828c328 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c >@@ -0,0 +1,20 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ >+/* { dg-final { scan-assembler-not "%lo\\(a_d\\+4\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ Why is this even generating SW? GCC should know these are unaligned and not use SW. That is a separate issue though. Did these %lo parts without an offset have to fall into the same category as %lo with an offset due to implementation? Given there is no offset then the %hi/%lo cannot mismatch. Simply forcing all unaligned address calculations to a register might be the most elegant solution but if so then it needs commenting on in the mips_lo_sum_valid_p function or someone will try to optimise this later and break something else. >+ >+double d; >+double __attribute__((aligned(1))) a_d; >+ >+float f; >+float __attribute__((aligned(1))) a_f; >+ >+void foo (void) >+{ >+ d = 0; >+ f = 0; >+ a_d = 0; >+ a_f = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c >new file mode 100644 >index 0000000..2eb8f9a >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c >@@ -0,0 +1,25 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ >+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(c\\)" } } */ >+/* { dg-final { scan-assembler "\tsh\t\\\$\[0-9\],%lo\\(s\\)" } } */ >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(i\\)" } } */ >+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(a_c\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsh\t\\\$\[0-9\],%lo\\(a_s\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_i\\)" } } */ >+ >+char __attribute__((aligned(1))) a_c; >+short __attribute__((aligned(1))) a_s; >+int __attribute__((aligned(1))) a_i; >+char c; >+short s; >+int i; >+ >+void foo (void) >+{ >+ c = 0; >+ s = 0; >+ i = 0; >+ a_c = 0; >+ a_s = 0; >+ a_i = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c >new file mode 100644 >index 0000000..8f348fb >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c >@@ -0,0 +1,16 @@ >+/* { dg-options "-mcode-readable=no -mabi=32 -mfp32 isa=1" } */ >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ >+ >+struct __attribute__((packed)) >+{ >+ short s; >+ double d; >+ float f; >+} h; >+ >+void foo (void) >+{ >+ h.d = 0; >+ h.f = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c >new file mode 100644 >index 0000000..9070b53 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c >@@ -0,0 +1,20 @@ >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfp32 isa=1" } */ >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ >+ >+double d; >+double __attribute__((aligned(1))) a_d; >+ >+float f; >+float __attribute__((aligned(1))) a_f; >+ >+void foo (void) >+{ >+ d = 0; >+ f = 0; >+ a_d = 0; >+ a_f = 0; >+} >+ >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c >new file mode 100644 >index 0000000..ee11366 >--- /dev/null >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c >@@ -0,0 +1,16 @@ >+/* { dg-options "-mcode-readable=no -mabi=32 -mfpxx isa=2" } */ >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ >+ >+struct __attribute__((packed)) >+{ >+ short s; >+ double d; >+ float f; >+} h; >+ >+void foo (void) >+{ >+ h.d = 0; >+ h.f = 0; >+} >+ > >
Hi Matthew, Many thanks for reviewing this patch. Upon closer validation of the patch I have found this this approach will not work in all cases so unfortunately I am going to have to abandon it. I will be shortly posting a new patch to fix this issue in the middle-end. Regards, Andrew > -----Original Message----- > From: Matthew Fortune > Sent: 05 May 2016 10:44 > To: Andrew Bennett; gcc-patches@gcc.gnu.org > Subject: RE: [PATCH] MIPS: Ensure that lo_sums do not contain an unaligned > symbol > > Hi Andrew, > > Thanks for working on this it is a painful area. There's a bit more to do > but this is cleaning up some sneaky bugs. Can you create a GCC bugzilla > entry if you haven't already as we should record where these bugs exist and > when they are fixed? > > See my comments but I think that you are fixing more variants of this bug > than your summary states so we need to capture the detail on what code is > affected by these issues. > > Andrew Bennett <Andrew.Bennett@imgtec.com> writes: > > different offsets. Lets show this with an example C program. > > > > struct > > { > > short s; > > unsigned long long l; > > } h; > > > > void foo (void) > > { > > h.l = 0; > > } > > > > When this is compiled for MIPS it produces the following assembly: > > > > lui $2,%hi(h+8) > > sw $0,%lo(h+8)($2) > > jr $31 > > sw $0,%lo(h+12)($2) > > This looks like a stale example h+8 implies 8 bytes of data preceding 'l'. > > > >diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > >index 6cdda3b..f07e433 100644 > >--- a/gcc/config/mips/mips.c > >+++ b/gcc/config/mips/mips.c > >@@ -2354,6 +2354,38 @@ mips_valid_lo_sum_p (enum mips_symbol_type > symbol_type, machine_mode mode) > > return true; > > } > > > >+/* Return true if X in LO_SUM (REG, X) is a valid. */ > > is a valid... > > I would however merge this code into mips_valid_lo_sum_p as the new function > name is fairly confusing. Both call sites for this function have the > symbol_type available which mips_valid_lo_sum_p requires and also have the > mode, reg, x so just add those to mips_valid_lo_sum_p. > > >+ > >+bool > >+mips_valid_lo_sum_lo_part_p (machine_mode mode, rtx reg, rtx x) > >+{ > >+ rtx symbol = NULL; > > three space indent. > > >+ > >+ if (mips_abi != ABI_32) > >+ return true; > > I don't think this is limited to o32. I was thinking this was just about > splitting a multi-word unaligned access but actually the test cases in this > patch show that it is also about accessing unaligned elements in a structure > using the same 'hi' part for multiple 'lo' parts with differing offsets. > > In the end I think there is no word size or abi specific issues here; it is > quite general. > > >+ if (mode == BLKmode) > >+ return true; > > Why is this special? Does the core GCC code ensure that lo_sum on a BLKmode > cannot have a constant offset greater than alignment? > > >+ if (reg && REG_P (reg) && REGNO (reg) == GLOBAL_POINTER_REGNUM) > >+ return true; > > I don't think reg need be an optional argument it is available at both > call sites. A comment to say why offsets from the global pointer are > not affected would also be useful. > > >+ > >+ if (GET_CODE (x) == CONST > >+ && GET_CODE (XEXP (x, 0)) == PLUS > >+ && GET_CODE (XEXP (XEXP (x, 0), 0)) == SYMBOL_REF) > >+ symbol = XEXP (XEXP (x, 0), 0); > >+ else if (GET_CODE (x) == SYMBOL_REF) > >+ symbol = x; > >+ > >+ if (symbol > >+ && SYMBOL_REF_DECL (symbol) > >+ && (GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT) > > >+ (DECL_ALIGN_UNIT (SYMBOL_REF_DECL (symbol)))) > > This needs another bracket to cover the multiline '>' condition. > > >+ return false; > >+ > >+ return true; > >+} > >+ > > /* Return true if X is a valid address for machine mode MODE. If it is, > > fill in INFO appropriately. STRICT_P is true if REG_OK_STRICT is in > > effect. */ > >@@ -2394,7 +2426,8 @@ mips_classify_address (struct mips_address_info *info, > rtx x, > > info->symbol_type > > = mips_classify_symbolic_expression (info->offset, SYMBOL_CONTEXT_MEM); > > return (mips_valid_base_register_p (info->reg, mode, strict_p) > >- && mips_valid_lo_sum_p (info->symbol_type, mode)); > >+ && mips_valid_lo_sum_p (info->symbol_type, mode) > >+ && mips_valid_lo_sum_lo_part_p (mode, info->reg, info->offset)); > > As above this can become: > > && mips_valid_lo_sum_p (info->symbol_type, mode, info-reg, > info->offset) > > > > > case CONST_INT: > > /* Small-integer addresses don't occur very often, but they > >@@ -3143,6 +3176,8 @@ mips_split_symbol (rtx temp, rtx addr, machine_mode > mode, rtx *low_out) > > high = gen_rtx_HIGH (Pmode, copy_rtx (addr)); > > high = mips_force_temporary (temp, high); > > *low_out = gen_rtx_LO_SUM (Pmode, high, addr); > >+ if (!mips_valid_lo_sum_lo_part_p (mode, NULL, addr)) > > tab indent. This can also become: > > if (!mips_valid_lo_sum_p (symbol_type, mode, high, addr)) > > >+ *low_out = mips_force_temporary (temp, *low_out); > > break; > > } > > return true; > > General comment on the tests... I don't think any of the '-mfp??' related > tests > are actually doing anything with the FPU. All the load/store operations are > happening in GP registers so the FP mode is irrelevant. This makes some > tests redundant. > > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c > >new file mode 100644 > >index 0000000..dc81fd9 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c > >@@ -0,0 +1,14 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > > Is the -mcode-readable=no so that these checks work for MIPS16? > > >+/* { dg-final { scan-assembler-not "%lo\\(h\\+6\\)" } } */ > >+ > >+struct __attribute__((packed)) > >+{ > >+ short s; > >+ unsigned long long l; > > indent 2 space. > > >+} h; > >+ > >+void foo (void) > > void > foo (void) > > throughout the tests. > > >+{ > >+ h.l = 0; > > indent 2 space. > > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c > >new file mode 100644 > >index 0000000..da80b67 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c > >@@ -0,0 +1,19 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfpxx isa=2" } */ > >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ > >+/* { dg-final { scan-assembler "%lo\\(f\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ > > This should be checking that there is no %lo(a_d+4) anywhere in the code. > > >+ > >+double d; > >+double __attribute__((aligned(1))) a_d; > >+float f; > >+float __attribute__((aligned(1))) a_f; > >+ > >+void foo (void) > >+{ > >+ d = 0; > >+ f = 0; > >+ a_d = 0; > >+ a_f = 0; > > indent 2 space. > > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c > >new file mode 100644 > >index 0000000..b2679f9 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c > >@@ -0,0 +1,17 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ > > This appears to be another form of the bug but is not mentioned in the > summary. Current GCC generates the %lo that must not appear in this > test. > > >+ > >+struct __attribute__((packed)) > >+{ > >+ char c; > >+ short s; > >+ int i; > > indent 2 space > > >+} h; > >+ > >+void foo (void) > >+{ > >+ h.c = 0; > >+ h.s = 0; > >+ h.i = 0; > > indent 2 space > > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c > >new file mode 100644 > >index 0000000..1b9a0b1 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c > >@@ -0,0 +1,13 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ > >+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ > >+ > >+unsigned long long l; > >+unsigned long long __attribute__((aligned(1))) a_l; > >+ > >+void foo (void) > >+{ > >+ l = 0; > >+ a_l = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c > >new file mode 100644 > >index 0000000..a01c1d8 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c > >@@ -0,0 +1,15 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ > >+/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ > > Why not check 'd' and 'a_d' as well? > > >+ > >+long long l; > >+double d; > >+ > >+long long __attribute__((aligned(1))) a_l; > >+double __attribute__((aligned(1))) a_d; > >+ > >+void foo (void) > >+{ > >+ d = (double) l; > >+ a_d = (double) a_l; > > indent 2 space. > > >+} > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c > >new file mode 100644 > >index 0000000..496c078 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c > >@@ -0,0 +1,16 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ > >+ > >+struct __attribute__((packed)) > >+{ > >+ short s; > >+ double d; > >+ float f; > > indent 2 space... throughout, I won't mark each one from here on. > > >+} h; > >+ > >+void foo (void) > >+{ > >+ h.d = 0; > >+ h.f = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c > >new file mode 100644 > >index 0000000..828c328 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c > >@@ -0,0 +1,20 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ > >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ > >+/* { dg-final { scan-assembler-not "%lo\\(a_d\\+4\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ > > Why is this even generating SW? GCC should know these are unaligned and > not use SW. That is a separate issue though. > > Did these %lo parts without an offset have to fall into the same category > as %lo with an offset due to implementation? Given there is no offset > then the %hi/%lo cannot mismatch. Simply forcing all unaligned address > calculations to a register might be the most elegant solution but if so > then it needs commenting on in the mips_lo_sum_valid_p function or > someone will try to optimise this later and break something else. > > >+ > >+double d; > >+double __attribute__((aligned(1))) a_d; > >+ > >+float f; > >+float __attribute__((aligned(1))) a_f; > >+ > >+void foo (void) > >+{ > >+ d = 0; > >+ f = 0; > >+ a_d = 0; > >+ a_f = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c > >new file mode 100644 > >index 0000000..2eb8f9a > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c > >@@ -0,0 +1,25 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ > >+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(c\\)" } } */ > >+/* { dg-final { scan-assembler "\tsh\t\\\$\[0-9\],%lo\\(s\\)" } } */ > >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(i\\)" } } */ > >+/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(a_c\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsh\t\\\$\[0-9\],%lo\\(a_s\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_i\\)" } } */ > >+ > >+char __attribute__((aligned(1))) a_c; > >+short __attribute__((aligned(1))) a_s; > >+int __attribute__((aligned(1))) a_i; > >+char c; > >+short s; > >+int i; > >+ > >+void foo (void) > >+{ > >+ c = 0; > >+ s = 0; > >+ i = 0; > >+ a_c = 0; > >+ a_s = 0; > >+ a_i = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c > >new file mode 100644 > >index 0000000..8f348fb > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c > >@@ -0,0 +1,16 @@ > >+/* { dg-options "-mcode-readable=no -mabi=32 -mfp32 isa=1" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ > >+ > >+struct __attribute__((packed)) > >+{ > >+ short s; > >+ double d; > >+ float f; > >+} h; > >+ > >+void foo (void) > >+{ > >+ h.d = 0; > >+ h.f = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c > >new file mode 100644 > >index 0000000..9070b53 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c > >@@ -0,0 +1,20 @@ > >+/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfp32 isa=1" } */ > >+/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ > >+/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ > >+/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ > >+ > >+double d; > >+double __attribute__((aligned(1))) a_d; > >+ > >+float f; > >+float __attribute__((aligned(1))) a_f; > >+ > >+void foo (void) > >+{ > >+ d = 0; > >+ f = 0; > >+ a_d = 0; > >+ a_f = 0; > >+} > >+ > >diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c > b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c > >new file mode 100644 > >index 0000000..ee11366 > >--- /dev/null > >+++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c > >@@ -0,0 +1,16 @@ > >+/* { dg-options "-mcode-readable=no -mabi=32 -mfpxx isa=2" } */ > >+/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ > >+ > >+struct __attribute__((packed)) > >+{ > >+ short s; > >+ double d; > >+ float f; > >+} h; > >+ > >+void foo (void) > >+{ > >+ h.d = 0; > >+ h.f = 0; > >+} > >+ > > > >
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 6cdda3b..f07e433 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -2354,6 +2354,38 @@ mips_valid_lo_sum_p (enum mips_symbol_type symbol_type, machine_mode mode) return true; } +/* Return true if X in LO_SUM (REG, X) is a valid. */ + +bool +mips_valid_lo_sum_lo_part_p (machine_mode mode, rtx reg, rtx x) +{ + rtx symbol = NULL; + + if (mips_abi != ABI_32) + return true; + + if (mode == BLKmode) + return true; + + if (reg && REG_P (reg) && REGNO (reg) == GLOBAL_POINTER_REGNUM) + return true; + + if (GET_CODE (x) == CONST + && GET_CODE (XEXP (x, 0)) == PLUS + && GET_CODE (XEXP (XEXP (x, 0), 0)) == SYMBOL_REF) + symbol = XEXP (XEXP (x, 0), 0); + else if (GET_CODE (x) == SYMBOL_REF) + symbol = x; + + if (symbol + && SYMBOL_REF_DECL (symbol) + && (GET_MODE_ALIGNMENT (mode) / BITS_PER_UNIT) > + (DECL_ALIGN_UNIT (SYMBOL_REF_DECL (symbol)))) + return false; + + return true; +} + /* Return true if X is a valid address for machine mode MODE. If it is, fill in INFO appropriately. STRICT_P is true if REG_OK_STRICT is in effect. */ @@ -2394,7 +2426,8 @@ mips_classify_address (struct mips_address_info *info, rtx x, info->symbol_type = mips_classify_symbolic_expression (info->offset, SYMBOL_CONTEXT_MEM); return (mips_valid_base_register_p (info->reg, mode, strict_p) - && mips_valid_lo_sum_p (info->symbol_type, mode)); + && mips_valid_lo_sum_p (info->symbol_type, mode) + && mips_valid_lo_sum_lo_part_p (mode, info->reg, info->offset)); case CONST_INT: /* Small-integer addresses don't occur very often, but they @@ -3143,6 +3176,8 @@ mips_split_symbol (rtx temp, rtx addr, machine_mode mode, rtx *low_out) high = gen_rtx_HIGH (Pmode, copy_rtx (addr)); high = mips_force_temporary (temp, high); *low_out = gen_rtx_LO_SUM (Pmode, high, addr); + if (!mips_valid_lo_sum_lo_part_p (mode, NULL, addr)) + *low_out = mips_force_temporary (temp, *low_out); break; } return true; diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c new file mode 100644 index 0000000..dc81fd9 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset1.c @@ -0,0 +1,14 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler-not "%lo\\(h\\+6\\)" } } */ + +struct __attribute__((packed)) +{ + short s; + unsigned long long l; +} h; + +void foo (void) +{ + h.l = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c new file mode 100644 index 0000000..da80b67 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset10.c @@ -0,0 +1,19 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfpxx isa=2" } */ +/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ +/* { dg-final { scan-assembler "%lo\\(f\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ + +double d; +double __attribute__((aligned(1))) a_d; +float f; +float __attribute__((aligned(1))) a_f; + +void foo (void) +{ + d = 0; + f = 0; + a_d = 0; + a_f = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c new file mode 100644 index 0000000..b2679f9 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset11.c @@ -0,0 +1,17 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ + +struct __attribute__((packed)) +{ + char c; + short s; + int i; +} h; + +void foo (void) +{ + h.c = 0; + h.s = 0; + h.i = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c new file mode 100644 index 0000000..1b9a0b1 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset2.c @@ -0,0 +1,13 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ +/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ + +unsigned long long l; +unsigned long long __attribute__((aligned(1))) a_l; + +void foo (void) +{ + l = 0; + a_l = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c new file mode 100644 index 0000000..a01c1d8 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset3.c @@ -0,0 +1,15 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler-not "%lo\\(a_l\\+4\\)" } } */ +/* { dg-final { scan-assembler "%lo\\(l\\+4\\)" } } */ + +long long l; +double d; + +long long __attribute__((aligned(1))) a_l; +double __attribute__((aligned(1))) a_d; + +void foo (void) +{ + d = (double) l; + a_d = (double) a_l; +} diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c new file mode 100644 index 0000000..496c078 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset4.c @@ -0,0 +1,16 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ + +struct __attribute__((packed)) +{ + short s; + double d; + float f; +} h; + +void foo (void) +{ + h.d = 0; + h.f = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c new file mode 100644 index 0000000..828c328 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset5.c @@ -0,0 +1,20 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ +/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ +/* { dg-final { scan-assembler-not "%lo\\(a_d\\+4\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ + +double d; +double __attribute__((aligned(1))) a_d; + +float f; +float __attribute__((aligned(1))) a_f; + +void foo (void) +{ + d = 0; + f = 0; + a_d = 0; + a_f = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c new file mode 100644 index 0000000..2eb8f9a --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset6.c @@ -0,0 +1,25 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32" } */ +/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(c\\)" } } */ +/* { dg-final { scan-assembler "\tsh\t\\\$\[0-9\],%lo\\(s\\)" } } */ +/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(i\\)" } } */ +/* { dg-final { scan-assembler "\tsb\t\\\$\[0-9\],%lo\\(a_c\\)" } } */ +/* { dg-final { scan-assembler-not "\tsh\t\\\$\[0-9\],%lo\\(a_s\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_i\\)" } } */ + +char __attribute__((aligned(1))) a_c; +short __attribute__((aligned(1))) a_s; +int __attribute__((aligned(1))) a_i; +char c; +short s; +int i; + +void foo (void) +{ + c = 0; + s = 0; + i = 0; + a_c = 0; + a_s = 0; + a_i = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c new file mode 100644 index 0000000..8f348fb --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset7.c @@ -0,0 +1,16 @@ +/* { dg-options "-mcode-readable=no -mabi=32 -mfp32 isa=1" } */ +/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ + +struct __attribute__((packed)) +{ + short s; + double d; + float f; +} h; + +void foo (void) +{ + h.d = 0; + h.f = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c new file mode 100644 index 0000000..9070b53 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset8.c @@ -0,0 +1,20 @@ +/* { dg-options "-mcode-readable=no -mno-gpopt -mabi=32 -mfp32 isa=1" } */ +/* { dg-final { scan-assembler "%lo\\(d\\+4\\)" } } */ +/* { dg-final { scan-assembler "\tsw\t\\\$\[0-9\],%lo\\(f\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_d\\)" } } */ +/* { dg-final { scan-assembler-not "\tsw\t\\\$\[0-9\],%lo\\(a_f\\)" } } */ + +double d; +double __attribute__((aligned(1))) a_d; + +float f; +float __attribute__((aligned(1))) a_f; + +void foo (void) +{ + d = 0; + f = 0; + a_d = 0; + a_f = 0; +} + diff --git a/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c new file mode 100644 index 0000000..ee11366 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/hi-lo-reloc-offset9.c @@ -0,0 +1,16 @@ +/* { dg-options "-mcode-readable=no -mabi=32 -mfpxx isa=2" } */ +/* { dg-final { scan-assembler-not "%lo\\(h\\+\[1-9\]\\)" } } */ + +struct __attribute__((packed)) +{ + short s; + double d; + float f; +} h; + +void foo (void) +{ + h.d = 0; + h.f = 0; +} +
Hi, In MIPS (and similarly for other RISC architectures) to load an absolute address of an object requires a two instruction sequence: one instruction to load the high part of the object's address, and one instruction to load the low part of the object's address. Typically the result from the calculation of the high part of the address will only be used by one instruction to load the low part of the address. However, in certain situations (for example when loading or storing double word values) the result of computing the high part of the address can be used by multiple instructions to load the low parts of an address at different offsets. Lets show this with an example C program. struct { short s; unsigned long long l; } h; void foo (void) { h.l = 0; } When this is compiled for MIPS it produces the following assembly: lui $2,%hi(h+8) sw $0,%lo(h+8)($2) jr $31 sw $0,%lo(h+12)($2) ... .globl h .section .bss,"aw",@nobits .align 3 .type h, @object .size h, 16 h: .space 16 Notice here that the high part of the address of object h is loaded into register $2, and this is then used as part of the low part calculation by two the sw instructions which each have different offsets. In MIPS the value of a low part calculation is treated as a signed value. It is therefore valid to use the result of a high part calculation with multiple low part calculations containing different offsets so long as when adding the result of the high part to the each of the sign extended low parts we get valid addresses. However, if we add the packed attribute to the h structure, the fields of the structure will not be naturally aligned and we can break the previous condition. Lets explain this in more detail with the following C program. struct __attribute__((packed)) { short s; unsigned long long l; } h; void foo (void) { h.l = 0; } When this is compiled for MIPS it produces the following assembly: lui $2,%hi(h) addiu $4,$2,%lo(h+2) addiu $3,$2,%lo(h+6) swl $0,3($4) swr $0,%lo(h+2)($2) swl $0,3($3) jr $31 swr $0,%lo(h+6)($2) ... .globl h .section .bss,"aw",@nobits .align 2 .type h, @object .size h, 10 h: .space 10 There are two things to highlight here. Firstly the alignment of the h structure has decreased from 8 bytes to 4 bytes. Secondly we have a low part calculation which adds an offset of 6 to the address of the h structure which is greater than its alignment. When the MIPS linker resolves a HI relocation (i.e. %hi(h)) it finds the next LO relocation (i.e. %lo(h+2)) in the relocation table and using the information from both of these relocations it computes the object's address and extracts its high part. Then, when the MIPS linker resolves a LO relocation it adds the offset to the object's address and then extracts the low part. Lets assume that object h has an address of 0x80007ffc. When the MIPS linker resolves the value of the HI relocation for object h, it will also use the value of the LO relocation for object h with an offset of 2. The high part value is therefore: HIGH (0x80007ffc + 2) = HIGH (0x80007ffe) = 0x8000 Then the MIPS linker resolves the value of LO relocation for object h with an offset of 2: LO (0x80007ffc + 2) = LO (0x80007ffe) = 0x7ffe Finally the MIPS linker resolves the value of the LO relocation for object h with an offset of 6: LO (0x80007ffc + 6) = LO (0x80008002) = 0x8002 In MIPS the value of a LO relocation is treated as a signed value, so when the program is run the address of h+6 will be 0x7fff8002 when it should be 0x80008002. To fix this issue I have changed the mips_split_symbol function in the case when it generates a set of instructions to compute the high and low part of a symbol's address. If the symbol is unaligned the low-part calculation is forced into a register. I have also added a condition into mips_classify_address that prevents lo_sums from being used if they contain an unaligned symbol. This stops GCC from trying to merge the result of a lo_sum that is currently in register back into an address calculation. I have tested the patch on the mips-mti-elf toolchain and there have been no regressions. The patch and ChangeLog are below. Ok to commit? Many thanks, Andrew gcc/ * config/mips/mips.c (mips_valid_lo_sum_p): New function. (mips_classify_address): Call mips_valid_lo_sum_p to check if we have a valid lo_sum. (mips_split_symbol): Force the lo_sum to a register if mips_valid_lo_sum_p is false. testsuite/ * gcc.target/mips/hi-lo-reloc-offset1: New test. * gcc.target/mips/hi-lo-reloc-offset2: New test. * gcc.target/mips/hi-lo-reloc-offset3: New test. * gcc.target/mips/hi-lo-reloc-offset4: New test. * gcc.target/mips/hi-lo-reloc-offset5: New test. * gcc.target/mips/hi-lo-reloc-offset6: New test. * gcc.target/mips/hi-lo-reloc-offset7: New test. * gcc.target/mips/hi-lo-reloc-offset8: New test. * gcc.target/mips/hi-lo-reloc-offset9: New test. * gcc.target/mips/hi-lo-reloc-offset10: New test. * gcc.target/mips/hi-lo-reloc-offset11: New test.