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.benn...@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;
> >+}
> >+
> >
> >
