On Thu, 10 Jan 2013, Eric Botcazou wrote:
> > The problem with this intermediate inconsistency is that ...
> >
> > > That's why only the final result should matter and need be correct.
> >
> > ... this can't be ensured anymore. In some cases the inconsistency
> > between the mem attributes and what XEXP(MEM, 0) represents can't be
> > repaired by the later bitpos adjustments, or better it can be only be
> > repaired by falling back to the conservative side for e.g. the alignment,
> > because we don't store enough information in the mem attributes to recover
> > what was lost.
>
> Indeed, looking at the history of the code shows that the alignment handling
> via MEM_REF and get_object_alignment is an afterthought. Originally, it was
> set only in a few specific cases:
>
> /* We can set the alignment from the type if we are making an object,
> this is an INDIRECT_REF, or if TYPE_ALIGN_OK. */
>
> and also for DECL_P and CONSTANT_CLASS_P. That was it, so this was actually
> the alignment of the base and disregarding BITPOS was fine.
Yes, in these cases bitpos is zero. It's wrong in the !align_computed
path and possibly in the ARRAY_REF -> DECL_P path (only not as
optimistic as it could be).
> > When briefly discussing this yesterday I suggested (without having the
> > code in front of me) that it be best to simply set the mem attributes only
> > at the very end, after having computed to final MEM address including all
> > adjustments, instead of generating wrong mem attributes initially and
> > hoping for the adjustments to make it come out right at the end.
>
> Maybe, but for 4.7 (and probably 4.8) I think we should be conservative and
> only fix the problems recently introduced.
>
> So, in the end, I think that we should go either for Richard's initial patch
> in this thread, possibly with a ??? comment along the lines of:
>
> /* ??? If we haven't computed the alignment yet, compute it now. Note
> that, if already computed above, the alignment is that of the base
> object of T so it is OK to have disregarded BITPOS above. However,
> here we are using get_object_alignment_1 which returns the precise
> alignment of T so we need to account for the BITPOS adjustment. */
> if (!align_computed)
>
> or for Richard's initial patch + the removal of the MEM_REF block (which is
> midway between Richard's initial patch and Richard's last patch).
Thus I'm preparing a patch that applies to both 4.7 and 4.8 at this
point, leaving some holes I see open (which are not to be closed in
the same way due to differences in behavior of get_object_alignment
on the 4.7 branch vs. trunk).
Bootstrap and regtest on x86_64-unknown-linux-gnu running, Eric,
can you give this a strict-align target testing on the 4.7 branch?
Martin, can you re-test this on sparc (it's slightly different again)?
I'm going to remove the MEM_REF block as a followup (we can't backport
that due to pessimizations this will cause due to different
get_object_alignment behavior - yet, unless we backport that change
as well), it's a source of wrong alignment as well, due to the
usage of MAX () - the get_object_alignment result will never shrink
alignment this way (and thus implicitely relies on attrs.align
being BITS_PER_UNIT when !align_computed).
The get_object_alignment behavior change is
2012-07-18 Richard Guenther <rguent...@suse.de>
* tree.h (get_object_or_type_alignment): Remove.
* builtins.c (get_object_alignment_2): New function copied from
get_object_alignment_1. Take extra argument to indicate whether
we take the address of EXP. Rework to use type alignment
information
if not, and return whether the result is an approximation or not.
(get_object_alignment_1): Wrap around get_object_alignment_2.
(get_pointer_alignment_1): Call get_object_alignment_2 indicating
we take the address.
(get_object_or_type_alignment): Remove.
* expr.c (expand_assignment): Call get_object_alignment.
(expand_expr_real_1): Likewise.
and followups.
Thanks,
Richard.
2013-01-11 Richard Biener <rguent...@suse.de>
PR middle-end/55882
* emit-rtl.c (set_mem_attributes_minus_bitpos): Correctly
account for bitpos when computing alignment.
* gcc.dg/torture/pr55882.c: New testcase.
Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c (revision 195102)
--- gcc/emit-rtl.c (working copy)
*************** set_mem_attributes_minus_bitpos (rtx ref
*** 1839,1845 ****
if (!align_computed)
{
! unsigned int obj_align = get_object_alignment (t);
attrs.align = MAX (attrs.align, obj_align);
}
}
--- 1839,1850 ----
if (!align_computed)
{
! unsigned int obj_align;
! unsigned HOST_WIDE_INT obj_bitpos;
! get_object_alignment_1 (t, &obj_align, &obj_bitpos);
! obj_bitpos = (obj_bitpos - bitpos) & (obj_align - 1);
! if (obj_bitpos != 0)
! obj_align = (obj_bitpos & -obj_bitpos);
attrs.align = MAX (attrs.align, obj_align);
}
}
Index: gcc/testsuite/gcc.dg/torture/pr55882.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr55882.c (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr55882.c (working copy)
***************
*** 0 ****
--- 1,94 ----
+ /* { dg-do run } */
+
+ typedef enum
+ {
+ PVT_A = 0,
+ PVT_B = 1,
+ PVT_CONFIG = 2,
+ PVT_RESERVED3 = 3,
+ } T_CR_SELECT;
+
+ typedef enum
+ {
+ STD_ULOGIC_0 = 0,
+ STD_ULOGIC_1 = 1,
+ } STD_ULOGIC;
+
+ typedef struct
+ {
+ unsigned char rtp : 3;
+ unsigned char rtn : 3;
+ } C;
+
+ typedef struct
+ {
+ unsigned char nd;
+ unsigned char pd;
+ unsigned char rtn;
+ unsigned char rtp;
+ } A;
+
+ typedef struct
+ {
+ unsigned short reserved : 14;
+ unsigned char Z_rx_enable : 2;
+ A pvt;
+ } B;
+
+ typedef struct
+ {
+ B cr_dsclk_q3;
+ B cr_data_q3;
+ B cr_addr_q3;
+ B cr_cmd_q3;
+ B cr_pres_q3;
+ C cr_vref_q3[6];
+ unsigned char pres_disable;
+ unsigned char pres_drive_high;
+ unsigned char c_enab_120;
+ STD_ULOGIC clk_tximp;
+ STD_ULOGIC dqs_tximp;
+ STD_ULOGIC cmd_tximp;
+ STD_ULOGIC data_tximp;
+ STD_ULOGIC dqs_rxterm;
+ STD_ULOGIC data_rxterm;
+ T_CR_SELECT cr_clk_sel;
+ unsigned char cr_clk : 5;
+ T_CR_SELECT cr_dsclk_odd_sel;
+ unsigned char cr_dsclk_odd : 5;
+ T_CR_SELECT cr_dsclk_even_sel;
+ unsigned char cr_dsclk_even : 5;
+ T_CR_SELECT cr_data_sel;
+ unsigned char cr_data : 5;
+ T_CR_SELECT cr_vref_sel;
+ unsigned char cr_vref : 5;
+ T_CR_SELECT cr_others_sel;
+ unsigned char cr_others : 5;
+ } CONFIG;
+
+ typedef struct
+ {
+ unsigned char enable_monitor;
+ unsigned short step_out_pointer : 12;
+ unsigned short hold_out_pointer : 12;
+ unsigned short enable_wr_dqs : 12;
+ unsigned short use_alt_rd_dqs : 12;
+ CONFIG io_buf;
+ } mystruct;
+
+ unsigned short __attribute__((noinline,noclone))
+ testfunction(unsigned i)
+ {
+ mystruct dmfe[8];
+ dmfe[0].use_alt_rd_dqs = 1;
+ dmfe[i].use_alt_rd_dqs = 0;
+ return dmfe[0].use_alt_rd_dqs;
+ }
+
+ extern void abort (void);
+ int main ()
+ {
+ if (testfunction(0) != 0)
+ abort ();
+ return 0;
+ }
