On Fri, 11 Jan 2013, Richard Biener wrote:
> 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.
I have applied the minimal fix to trunk now. We retain the clearly
incorrect fact that the !align_computed path only ever increases
alignment (it's only ever set previously to either BITS_PER_UNIT
or via the MEM_REF block). I will momentarily test a followup
removing that block for trunk.
Thanks,
Richard.
> 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;
> + }
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
