[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Martin Jambor jamborm at gcc dot gnu.org changed: What|Removed |Added URL||http://gcc.gnu.org/ml/gcc-p ||atches/2013-08/msg00224.htm ||l --- Comment #29 from Martin Jambor jamborm at gcc dot gnu.org --- Richi approved the patch, I've committed it to trunk. I'm about to bootstrap and test n the 4.8 branch. Author: jamborm Date: Tue Aug 6 09:22:16 2013 New Revision: 201523 URL: http://gcc.gnu.org/viewcvs?rev=201523root=gccview=rev Log: 2013-08-06 Martin Jambor mjam...@suse.cz PR middle-end/58041 * gimple-ssa-strength-reduction.c (replace_ref): Make sure built MEM_REF has proper alignment information. testsuite/ * gcc.dg/torture/pr58041.c: New test. * gcc.target/arm/pr58041.c: Likewise. Added: trunk/gcc/testsuite/gcc.dg/torture/pr58041.c trunk/gcc/testsuite/gcc.target/arm/pr58041.c Modified: trunk/gcc/ChangeLog trunk/gcc/gimple-ssa-strength-reduction.c trunk/gcc/testsuite/ChangeLog
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #30 from Bernd Edlinger bernd.edlinger at hotmail dot de --- Hi Martin, I have bootstrapped this patch for i686-pc-linux-gnu and have seen some excess errors in your test script: /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c: In function 'foo': /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: note: The ABI for passing parameters with 16-byte alignment has changed in GCC 4.6 /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] output is: /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c: In function 'foo': /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: note: The ABI for passing parameters with 16-byte alignment has changed in GCC 4.6 /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] FAIL: gcc.dg/torture/pr58041.c -O0 (test for excess errors) Excess errors: /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default]
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #31 from Martin Jambor jamborm at gcc dot gnu.org --- (In reply to Bernd Edlinger from comment #30) Hi Martin, I have bootstrapped this patch for i686-pc-linux-gnu and have seen some excess errors in your test script: /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c: In function 'foo': /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: note: The ABI for passing parameters with 16-byte alignment has changed in GCC 4.6 /home/ed/gnu/gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c:15:11: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] I can't reproduce this with the -m32 flag on my x86_64... do you still have the compiler built on an i686? If so, could you try and make function foo static in that testcase and see if the error goes away? Thanks!
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #32 from Martin Jambor jamborm at gcc dot gnu.org --- Author: jamborm Date: Tue Aug 6 15:08:59 2013 New Revision: 201530 URL: http://gcc.gnu.org/viewcvs?rev=201530root=gccview=rev Log: 2013-08-06 Martin Jambor mjam...@suse.cz PR middle-end/58041 * gimple-ssa-strength-reduction.c (replace_ref): Make sure built MEM_REF has proper alignment information. testsuite/ * gcc.dg/torture/pr58041.c: New test. * gcc.target/arm/pr58041.c: Likewise. Added: branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/torture/pr58041.c branches/gcc-4_8-branch/gcc/testsuite/gcc.target/arm/pr58041.c Modified: branches/gcc-4_8-branch/gcc/ChangeLog branches/gcc-4_8-branch/gcc/gimple-ssa-strength-reduction.c branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #33 from Bernd Edlinger bernd.edlinger at hotmail dot de --- (In reply to Martin Jambor from comment #31) I can't reproduce this with the -m32 flag on my x86_64... do you still have the compiler built on an i686? If so, could you try and make function foo static in that testcase and see if the error goes away? static does not help. If I add -msse the warning goes away, but the compiled executable crashes because of illegal instruction. Dual Pentium II, with mmx but obviously no sse, whatever that may be: flags: fpu vme de pse tsc msr pae mce cx8 apic mtrr pge mca cmov pse36 mmx fxsr
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #34 from Bernd Edlinger bernd.edlinger at hotmail dot de ---
by the way the initializer of struct s a =
seems to generate warnings at -Wall, because some brackets are missing:
changed that to
struct s a = {0,{{0,0},{0,0}}};
but somehow I wonder what forced us to generate sse instructions here?
when that same example works on a ARMv5 targe?
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #35 from Martin Jambor jamborm at gcc dot gnu.org ---
(In reply to Bernd Edlinger from comment #34)
by the way the initializer of struct s a =
seems to generate warnings at -Wall, because some brackets are missing:
changed that to
struct s a = {0,{{0,0},{0,0}}};
but somehow I wonder what forced us to generate sse instructions here?
when that same example works on a ARMv5 targe?
Strange, does the correct initializer make the warning go away?
If so, I'll fix it in the testsuite in a moment.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #36 from Bernd Edlinger bernd.edlinger at hotmail dot de ---
(In reply to Martin Jambor from comment #35)
(In reply to Bernd Edlinger from comment #34)
by the way the initializer
of struct s a =
seems to generate warnings at -Wall, because some
brackets are missing:
changed that to
struct s a = {0,{{0,0},{0,0}}};
but somehow I wonder what forced us to generate sse instructions here?
when that same example works on a ARMv5 targe?
Strange, does the correct
initializer make the warning go away?
If so, I'll fix it in the testsuite in a moment.
no that is just a different warning with -Wall, that one did not
make the test case fail however.
and in line 6 the typedef struct S { V v; } P __attribute__((aligned (1)));
is superfluos too.
hmm, maybe the problem is I should not say -msse in the first place.
do you get the warning if you use -m32 -mno-sse ?
what's funny about that warning, that it does not need to be enabled with
-Wall like the other warning.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #37 from Bernd Edlinger bernd.edlinger at hotmail dot de ---
this version fixes the warning:
--- ../gcc-4.9-20130728/gcc/testsuite/gcc.dg/torture/pr58041.c 2013-08-02
20:59:38.0 +0200
+++ pr58041.c 2013-08-06 18:30:51.0 +0200
@@ -3,8 +3,6 @@
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
-typedef struct S { V v; } P __attribute__((aligned (1)));
-
struct s
{
char u;
@@ -12,24 +10,24 @@
} __attribute__((packed,aligned(1)));
__attribute__((noinline, noclone))
-long long foo(struct s *x, int y, V z)
+long long foo(struct s *x, int y, V *z)
{
V a = x-v[y];
- x-v[y] = z;
+ x-v[y] = *z;
return a[1];
}
-struct s a = {0,{0,0}};
+struct s a = {0,{{0,0},{0,0}}};
int main()
{
V v1 = {0,1};
V v2 = {0,2};
- if (foo(a,0,v1) != 0)
+ if (foo(a,0,v1) != 0)
__builtin_abort();
- if (foo(a,0,v2) != 1)
+ if (foo(a,0,v2) != 1)
__builtin_abort();
- if (foo(a,1,v1) != 0)
+ if (foo(a,1,v1) != 0)
__builtin_abort();
return 0;
}
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #38 from Martin Jambor jamborm at gcc dot gnu.org --- (In reply to Bernd Edlinger from comment #37) this version fixes the warning: And I confirm that it still tests the bug. If you want to commit it yourself, go ahead, otherwise let me now and I'll do it before I leave today. Thanks a lot!
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #39 from Bernd Edlinger bernd.edlinger at hotmail dot de --- (In reply to Martin Jambor from comment #38) (In reply to Bernd Edlinger from comment #37) this version fixes the warning: And I confirm that it still tests the bug. If you want to commit it yourself, go ahead, otherwise let me now and I'll do it before I leave today. Thanks a lot! no thanks, just go ahead.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Martin Jambor jamborm at gcc dot gnu.org changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #40 from Martin Jambor jamborm at gcc dot gnu.org --- OK, I have updated the testcase on both branches. So I hope all is well now and we can close this PR. Thanks everyone for cooperation. Author: jamborm Date: Tue Aug 6 17:33:59 2013 New Revision: 201538 URL: http://gcc.gnu.org/viewcvs?rev=201538root=gccview=rev Log: 2013-08-06 Martin Jambor mjam...@suse.cz Bernd Edlinger bernd.edlin...@hotmail.de testsuite/ * gcc.dg/torture/pr58041.c (foo): Accept z by reference. (a): Fix constructor. Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/torture/pr58041.c Author: jamborm Date: Tue Aug 6 17:35:11 2013 New Revision: 201539 URL: http://gcc.gnu.org/viewcvs?rev=201539root=gccview=rev Log: 2013-08-06 Martin Jambor mjam...@suse.cz Bernd Edlinger bernd.edlin...@hotmail.de testsuite/ * gcc.dg/torture/pr58041.c (foo): Accept z by reference. (a): Fix constructor. Modified: branches/gcc-4_8-branch/gcc/testsuite/ChangeLog branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/torture/pr58041.c
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #41 from Bill Schmidt wschmidt at gcc dot gnu.org --- Thanks, Martin!
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #28 from Martin Jambor jamborm at gcc dot gnu.org --- Thanks, for testing, I have submitted the patch for a review: http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00224.html
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #27 from Bernd Edlinger bernd.edlinger at hotmail dot de --- (In reply to Martin Jambor from comment #24) Created attachment 30594 [details] Proposed patch I think it would be safe to put my initial test case under gcc/testsuite/gcc.target/arm/pr58041.c It passes with your patch at least in my environment.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #20 from Bill Schmidt wschmidt at gcc dot gnu.org --- After thinking it over some more, I think you are right, Martin. We should go ahead with the optimization with the corrected alignment attached to the type. Please go ahead with your patch. I will run a round of regression testing on PowerPC (an architecture for which the generic test produces misaligned but legal memory references) as well. Sorry for going back and forth on this. I try to avoid wasting compile time on useless transformations, but in this case we will still see some benefit in some cases, and the code should be no worse than before when we don't. Thanks, Bill
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Eric Botcazou ebotcazou at gcc dot gnu.org changed: What|Removed |Added CC||ebotcazou at gcc dot gnu.org --- Comment #22 from Eric Botcazou ebotcazou at gcc dot gnu.org --- After thinking it over some more, I think you are right, Martin. We should go ahead with the optimization with the corrected alignment attached to the type. Please go ahead with your patch. I will run a round of regression testing on PowerPC (an architecture for which the generic test produces misaligned but legal memory references) as well. Sorry for going back and forth on this. I try to avoid wasting compile time on useless transformations, but in this case we will still see some benefit in some cases, and the code should be no worse than before when we don't. We should be very wary of generating unaligned accesses during optimization for targets that define SLOW_UNALIGNED_ACCESS. And note that most architectures supported by GCC are STRICT_ALIGNMENT, not the other way around, so it's the common case.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #23 from Bill Schmidt wschmidt at gcc dot gnu.org --- (In reply to Eric Botcazou from comment #22) We should be very wary of generating unaligned accesses during optimization for targets that define SLOW_UNALIGNED_ACCESS. And note that most architectures supported by GCC are STRICT_ALIGNMENT, not the other way around, so it's the common case. I fully agree. In this case, we aren't introducing new unaligned accesses, but simply restructuring the address that points to the same (unaligned) location. The restructuring allows us to common the base addressing for related array elements in the same misaligned structure, which will produce slightly better code with the same number of unaligned accesses. Martin's patch just makes sure the necessary alignment is recorded on the restructured memory reference, so the back ends can do their usual tricks to copy misaligned references in parts, etc.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #21 from Bill Schmidt wschmidt at gcc dot gnu.org --- My only comment on the patch would be to please add commentary indicating why the change is being made, and referencing this PR. Something along the lines of: /* Ensure the memory reference carries the minimum alignment requirement for the data type. Some targets (e.g., ARM) can't always handle an unaligned reference otherwise. See PR58041. */ ...or something like that. Thanks again! Bill
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Martin Jambor jamborm at gcc dot gnu.org changed: What|Removed |Added Assignee|wschmidt at gcc dot gnu.org|jamborm at gcc dot gnu.org --- Comment #24 from Martin Jambor jamborm at gcc dot gnu.org --- Created attachment 30594 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30594action=edit Proposed patch (In reply to Bill Schmidt from comment #20) After thinking it over some more, I think you are right, Martin. We should go ahead with the optimization with the corrected alignment attached to the type. Please go ahead with your patch. I will run a round of regression testing on PowerPC (an architecture for which the generic test produces misaligned but legal memory references) as well. Sorry for going back and forth on this. I try to avoid wasting compile time on useless transformations, but in this case we will still see some benefit in some cases, and the code should be no worse than before when we don't. No worries, I'm currently bootstrapping and testing the attached patch. I'm bootstrapping on x86_64-linux and with bugs like this one, any additional testing on other platforms is very welcome. (In reply to Bill Schmidt from comment #21) My only comment on the patch would be to please add commentary indicating why the change is being made, and referencing this PR. Something along the lines of: /* Ensure the memory reference carries the minimum alignment requirement for the data type. Some targets (e.g., ARM) can't always handle an unaligned reference otherwise. See PR58041. */ ...or something like that. The alignment information should be there regardless of the target so I just used the first sentence and the PR reference. I hope that is enough. Thanks.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #25 from Bill Schmidt wschmidt at gcc dot gnu.org --- Yep, that's terrific. Thanks.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #26 from Bill Schmidt wschmidt at gcc dot gnu.org --- Martin's patch bootstrapped on powerpc64-unknown-linux-gnu with no new regressions.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #1 from Bernd Edlinger bernd.edlinger at hotmail dot de --- Created attachment 30579 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30579action=edit test case to show the bug
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #2 from Bernd Edlinger bernd.edlinger at hotmail dot de --- Sandra, this seems to be unrelated to your strict-volatile-bitfields patch, as it happens with or without that patch.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Mikael Pettersson mikpe at it dot uu.se changed: What|Removed |Added CC||mikpe at it dot uu.se --- Comment #3 from Mikael Pettersson mikpe at it dot uu.se --- Running this program throws loads of alignment exceptions when it's compiled by gcc-4.9 or gcc-4.8, targeting armv5tel-linux-gnueabi -O2 -marm. Adding -mno-unaligned-access makes no difference. When compiled by gcc-4.7 it runs cleanly without any exceptions.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
Mikael Pettersson mikpe at it dot uu.se changed:
What|Removed |Added
CC||wschmidt at gcc dot gnu.org
--- Comment #4 from Mikael Pettersson mikpe at it dot uu.se ---
Started with Bill Schmidt's PR46556 patch in r190037. (Author CC:d.)
Comparing the generated code between 190036 and 190037 clearly shows how the
misaligned accesses were wrongly replaced by aligned accesses:
--- pr58041.s-r190036 2013-08-01 13:30:59.264514025 +0200
+++ pr58041.s-r190037 2013-08-01 13:27:38.874840851 +0200
@@ -18,37 +18,11 @@
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
- stmfd sp!, {r4, r5, r6, r7, r8, r9, r10, fp}
add ip, r0, r1, asl #3
- ldrbr7, [ip, #2]@ zero_extendqisi2
- ldrbr6, [ip, #6]@ zero_extendqisi2
- ldrbr0, [ip, #1]@ zero_extendqisi2
- ldrbr1, [ip, #5]@ zero_extendqisi2
- ldrbr5, [ip, #3]@ zero_extendqisi2
- ldrbr4, [ip, #7]@ zero_extendqisi2
- orr r0, r0, r7, asl #8
- orr r1, r1, r6, asl #8
- ldrbr10, [ip, #4] @ zero_extendqisi2
- ldrbr6, [ip, #8]@ zero_extendqisi2
- mov fp, r2, lsr #8
- orr r0, r0, r5, asl #16
- mov r9, r2, lsr #16
- mov r8, r2, lsr #24
- mov r7, r3, lsr #8
- orr r1, r1, r4, asl #16
- mov r5, r3, lsr #16
- mov r4, r3, lsr #24
- strbfp, [ip, #2]
- strbr2, [ip, #1]
- strbr9, [ip, #3]
- strbr8, [ip, #4]
- strbr7, [ip, #6]
- strbr3, [ip, #5]
- strbr5, [ip, #7]
- strbr4, [ip, #8]
- orr r0, r0, r10, asl #24
- orr r1, r1, r6, asl #24
- ldmfd sp!, {r4, r5, r6, r7, r8, r9, r10, fp}
+ ldr r0, [ip, #1]
+ ldr r1, [ip, #5]
+ str r2, [ip, #1]
+ str r3, [ip, #5]
bx lr
.size foo, .-foo
.section.text.startup,ax,%progbits
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #5 from Mikael Pettersson mikpe at it dot uu.se --- I see the exact same failure pattern on sparc64-linux: 4.7 generates working code, 4.8 and 4.9 generate code that SIGBUS:es, failure starts with r190037, -m32 or -m64 makes no difference.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #6 from Bill Schmidt wschmidt at gcc dot gnu.org --- I'll investigate. It may be a day or two before I can get to it, but this is pretty clearly my issue. Thanks, Bill
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 Bill Schmidt wschmidt at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |ASSIGNED Last reconfirmed||2013-08-01 Assignee|unassigned at gcc dot gnu.org |wschmidt at gcc dot gnu.org Ever confirmed|0 |1 --- Comment #7 from Bill Schmidt wschmidt at gcc dot gnu.org --- This shouldn't be too hard to fix. Looks like we are missing a check for possibly unaligned data when STRICT_ALIGNMENT is set. The logic for unaligned data can be exposed from a static routine in tree-ssa-ivopts.c. I'll work up a trial patch.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #8 from Martin Jambor jamborm at gcc dot gnu.org --- I believe that you need to set alignment of the type of MEM_REFs you create in replace_ref along the lines it is done in build_ref_for_offset in tree-sra.c. I wonder whether STRICT_ALIGNMENT has really any meaning nowadays, given that for example x86_64 is not a strict-alignment platform except for SSE vectors.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #9 from Martin Jambor jamborm at gcc dot gnu.org ---
More specifically, if I am correct assuming that the MEM_REF
replace_ref builds always accesses exactly the same memory as the
previous access *expr does (and only the address is computed better)
and that *expr is how the function accessed that memory before, then I
think what you need is the following (untested, except that I know it
seems to fix the testcase, at least the assembly looks very different
:-)
*** /tmp/heQHTs_gimple-ssa-strength-reduction.c Thu Aug 1 18:43:45 2013
--- gcc/gimple-ssa-strength-reduction.c Thu Aug 1 18:38:31 2013
*** dump_incr_vec (void)
*** 1728,1738
static void
replace_ref (tree *expr, slsr_cand_t c)
{
! tree add_expr = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (c-base_expr),
! c-base_expr, c-stride);
! tree mem_ref = fold_build2 (MEM_REF, TREE_TYPE (*expr), add_expr,
! double_int_to_tree (c-cand_type, c-index));
!
/* Gimplify the base addressing expression for the new MEM_REF tree. */
gimple_stmt_iterator gsi = gsi_for_stmt (c-cand_stmt);
TREE_OPERAND (mem_ref, 0)
--- 1728,1748
static void
replace_ref (tree *expr, slsr_cand_t c)
{
! tree add_expr, mem_ref, acc_type = TREE_TYPE (*expr);
! unsigned HOST_WIDE_INT misalign;
! unsigned align;
!
! get_object_alignment_1 (*expr, align, misalign);
! if (misalign != 0)
! align = (misalign -misalign);
! if (align TYPE_ALIGN (acc_type))
! acc_type = build_aligned_type (acc_type, align);
!
! add_expr = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (c-base_expr),
! c-base_expr, c-stride);
! mem_ref = fold_build2 (MEM_REF, acc_type, add_expr,
!double_int_to_tree (c-cand_type, c-index));
!
/* Gimplify the base addressing expression for the new MEM_REF tree. */
gimple_stmt_iterator gsi = gsi_for_stmt (c-cand_stmt);
TREE_OPERAND (mem_ref, 0)
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #10 from Martin Jambor jamborm at gcc dot gnu.org --- Created attachment 30587 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30587action=edit x86_64-linux testcase To prove the point, this is an x86_64-linux testcase. I will bootstrap and test the patch overnight and if it passes, I will submit it to the mailing list. Still, please confirm my assumptions outlined above are correct. Thanks.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041
--- Comment #11 from Bill Schmidt wschmidt at gcc dot gnu.org ---
Hi Martin,
Your assumptions are correct, but I'm not sure this is the best place to handle
it. It looks like what you are doing is replacing one already correct memory
reference with another, both of which will generate somewhat nasty code.
Therefore there isn't much reason to do the transformation at all in the first
place. I think I would rather analyze the reference when considering adding
the reference to the candidate table, and leaving it out of consideration
altogether. What do you think?
For example, I'm looking at adding the following ahead of the call to
restructure_reference in slsr_process_ref:
/* If this reference doesn't meet alignment restrictions, don't
make it a candidate. Logic similar to that in tree-ssa-loop-ivopts.c:
may_be_unaligned_p(), without the STEP check. */
if (mode != BLKmode)
{
tree base_type = TREE_TYPE (base);
unsigned base_align = get_object_alignment (base);
unsigned mode_align = GET_MODE_ALIGNMENT (mode);
base_align = MAX (base_align, TYPE_ALIGN (base_type));
if (base_align mode_align
|| (bitpos % mode_align) != 0
|| (bitpos % BITS_PER_UNIT) != 0)
return;
if (offset
(highest_pow2_factor (offset) * BITS_PER_UNIT) mode_align)
return;
}
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #12 from Bill Schmidt wschmidt at gcc dot gnu.org --- ...which apparently is not quite right, since the candidates still appear in the table. Hm. But you get the idea -- do the check earlier.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #13 from Bernd Edlinger bernd.edlinger at hotmail dot de --- Hi, just one question, how about the -m[no-]unaligned-access option? If -munaligned-access had been given the code was almost right, I mean AFAIK ldr/str should be handled in hardware but ldmia generates an alignment exception and _may_ be emulated by an IRQ handler, but that would not be very efficient. When -mno-unaligned-access is given any ldr/str on unaligned addresses have to be avoided.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #14 from Bill Schmidt wschmidt at gcc dot gnu.org --- (In reply to Bernd Edlinger from comment #13) Hi, just one question, how about the -m[no-]unaligned-access option? If -munaligned-access had been given the code was almost right, I mean AFAIK ldr/str should be handled in hardware but ldmia generates an alignment exception and _may_ be emulated by an IRQ handler, but that would not be very efficient. When -mno-unaligned-access is given any ldr/str on unaligned addresses have to be avoided. Well, unfortunately -mno-unaligned-access does not provide any information to the middle end. It's all handled in the ARM back end. So without directly checking an ARM-specific option in the middle-end (evil), we don't have a good solution for that. That's how I initially started looking at STRICT_ALIGNMENT, which ARM and Sparc have in common. However, since this is supposed to be an optimization and it is common for misaligned memory accesses to suffer a penalty, I think it is better just to not optimize when the memory access is misaligned, and leave it to the target back ends to do their normal cleanups and workarounds.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #15 from Bill Schmidt wschmidt at gcc dot gnu.org --- Bernd, Mikael, Martin: Could you please test this on your respective targets? Index: gcc/gimple-ssa-strength-reduction.c === --- gcc/gimple-ssa-strength-reduction.c(revision 201413) +++ gcc/gimple-ssa-strength-reduction.c(working copy) @@ -845,6 +845,8 @@ slsr_process_ref (gimple gs) int unsignedp, volatilep; double_int index; slsr_cand_t c; + unsigned HOST_WIDE_INT misalign; + unsigned align; if (gimple_vdef (gs)) ref_expr = gimple_assign_lhs (gs); @@ -857,6 +859,16 @@ slsr_process_ref (gimple gs) DECL_BIT_FIELD (TREE_OPERAND (ref_expr, 1 return; + /* If this reference doesn't meet alignment restrictions, don't + make it a candidate. */ + get_object_alignment_1 (ref_expr, align, misalign); + + if (misalign != 0) +align = (misalign -misalign); + + if (align TYPE_ALIGN (TREE_TYPE (ref_expr))) +return; + base = get_inner_reference (ref_expr, bitsize, bitpos, offset, mode, unsignedp, volatilep, false); index = double_int::from_uhwi (bitpos); Thanks, Bill
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #16 from Bernd Edlinger bernd.edlinger at hotmail dot de --- (In reply to Bill Schmidt from comment #15) Bernd, Mikael, Martin: Could you please test this on your respective targets? Congatulations! it works. If I compile with -mno-unaligned-access all accesses are ldrb/strb as it should be. And if I compile with -mcpu=cortex-a9 -munaligned-access the code is also OK, no ldmia's any more. The back end seems to have fixed that for us. foo: @ args = 0, pretend = 0, frame = 0 @ frame_needed = 0, uses_anonymous_args = 0 @ link register save eliminated. add ip, r0, r1, asl #3 ldr r0, [ip, #1]@ unaligned ldr r1, [ip, #5]@ unaligned str r2, [ip, #1]@ unaligned str r3, [ip, #5]@ unaligned bx lr which is perfectly OK for cortex-a9.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #17 from Bill Schmidt wschmidt at gcc dot gnu.org --- Excellent! Thanks for the confirmation.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #18 from Mikael Pettersson mikpe at it dot uu.se --- (In reply to Bill Schmidt from comment #15) Bernd, Mikael, Martin: Could you please test this on your respective targets? This patch eliminates the misalignment faults for me on both ARMv5TE and SPARC.
[Bug middle-end/58041] Unaligned access to arrays in packed structure
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58041 --- Comment #19 from Martin Jambor jamborm at gcc dot gnu.org --- (In reply to Bill Schmidt from comment #15) Bernd, Mikael, Martin: Could you please test this on your respective targets? Well, my target is x86_64 but yes, it works. (In reply to Bill Schmidt from comment #11) Hi Martin, Your assumptions are correct, but I'm not sure this is the best place to handle it. It looks like what you are doing is replacing one already correct memory reference with another, both of which will generate somewhat nasty code. Therefore there isn't much reason to do the transformation at all in the first place. I think I would rather analyze the reference when considering adding the reference to the candidate table, and leaving it out of consideration altogether. What do you think? I don't know, at least in theory the optimization might help somewhat anyway, especially on targets that can handle misalignment memory accesses. But you are right that generally misaligned access will be slow either way. Anyway, I don't really care, I assume you contributed the code so you are more qualified to make a judgment and if you prefer one way over the other, go for it. I'll leave it to you and won't submit any patch then. Please make sure that the two testcases are added to the testsuite before you close the bug. The x86_64-linux tetcase from comment #10 is generic enough that it can go to gcc.dg/torture/, the original ARM one needs to go to some arm-specific place.
