| Message ID | alpine.LNX.2.00.1301091254470.6889@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
> This fixes PR55882 - set_mem_attributes_minus_bitpos misses to > account for the to-be applied bitpos when computing MEM_ALIGN. > It extracts alignment from 't' instead of &t - bitpos. > > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap > and regtest running on mips. > > Does this look sensible? I'm not sure, bitpos isn't taken into account in the other cases when the alignment is computed. adjust_address_1 is supposed to properly adjust the alignment by the time the offset is applied.
On Wed, 9 Jan 2013, Eric Botcazou wrote: > > This fixes PR55882 - set_mem_attributes_minus_bitpos misses to > > account for the to-be applied bitpos when computing MEM_ALIGN. > > It extracts alignment from 't' instead of &t - bitpos. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap > > and regtest running on mips. > > > > Does this look sensible? > > I'm not sure, bitpos isn't taken into account in the other cases when the > alignment is computed. adjust_address_1 is supposed to properly adjust the > alignment by the time the offset is applied. Yes, but alignment adjustment assumes the alignment is that of the actual MEM, not that of the MEM_EXPR. Thus, MEM_ALIGN is the alignment of MEM_EXPR - MEM_OFFSET. It's true that the other paths setting align_computed have the same issue. I can produce a followup (or preparatory patch) that at least removes the code that is redundant. For example /* If this is a decl, set the attributes of the MEM from it. */ if (DECL_P (t)) { attrs.expr = t; attrs.offset_known_p = true; attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); attrs.align = DECL_ALIGN (t); align_computed = true; } the alignment computation can be handled by the get_object_alignment path just fine, no need to duplicate it here, likewise for the CONSTANT_CLASS_P case. The ARRAY_REF path is scary enough to me (I'd rather drop it completely ... it's probably designed to make more cases covered by nonoverlapping_component_refs_p?). At least handling of DECL/COMPONENT_REF below the ARRAY_REFs should be commonized - for example the COMPONENT_REF case else if (TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; if (host_integerp (off_tree, 1)) { attrs.offset_known_p = true; attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } doesn't adjust 't' nor does it compute alignment. So it clearly cannot be that MEM_ALIGN is supposed to be the alignment of MEM_EXPR. That is - the question boils down to _what_ should MEM_ALIGN be the alignment of? In all the rest of the compiler it surely is the alignment of XEXP (mem, 0) - to make it differ from that during a brief period of expand_assignment sounds fragile at least. I'll try to simplify code paths without changing behavior first. Thanks, Richard.
On Thu, 10 Jan 2013, Richard Biener wrote: > On Wed, 9 Jan 2013, Eric Botcazou wrote: > > > > This fixes PR55882 - set_mem_attributes_minus_bitpos misses to > > > account for the to-be applied bitpos when computing MEM_ALIGN. > > > It extracts alignment from 't' instead of &t - bitpos. > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap > > > and regtest running on mips. > > > > > > Does this look sensible? > > > > I'm not sure, bitpos isn't taken into account in the other cases when the > > alignment is computed. adjust_address_1 is supposed to properly adjust the > > alignment by the time the offset is applied. > > Yes, but alignment adjustment assumes the alignment is that of > the actual MEM, not that of the MEM_EXPR. Thus, MEM_ALIGN is the > alignment of MEM_EXPR - MEM_OFFSET. > > It's true that the other paths setting align_computed have the > same issue. I can produce a followup (or preparatory patch) > that at least removes the code that is redundant. For example > > /* If this is a decl, set the attributes of the MEM from it. */ > if (DECL_P (t)) > { > attrs.expr = t; > attrs.offset_known_p = true; > attrs.offset = 0; > apply_bitpos = bitpos; > new_size = DECL_SIZE_UNIT (t); > attrs.align = DECL_ALIGN (t); > align_computed = true; > } > > the alignment computation can be handled by the get_object_alignment > path just fine, no need to duplicate it here, likewise for the > CONSTANT_CLASS_P case. The ARRAY_REF path is scary enough to > me (I'd rather drop it completely ... it's probably designed to > make more cases covered by nonoverlapping_component_refs_p?). > At least handling of DECL/COMPONENT_REF below the ARRAY_REFs > should be commonized - for example the COMPONENT_REF case > > else if (TREE_CODE (t2) == COMPONENT_REF) > { > attrs.expr = t2; > attrs.offset_known_p = false; > if (host_integerp (off_tree, 1)) > { > attrs.offset_known_p = true; > attrs.offset = tree_low_cst (off_tree, 1); > apply_bitpos = bitpos; > } > > doesn't adjust 't' nor does it compute alignment. So it > clearly cannot be that MEM_ALIGN is supposed to be the > alignment of MEM_EXPR. > > That is - the question boils down to _what_ should MEM_ALIGN > be the alignment of? In all the rest of the compiler it surely > is the alignment of XEXP (mem, 0) - to make it differ from that > during a brief period of expand_assignment sounds fragile at least. > > I'll try to simplify code paths without changing behavior first. Going over this what's strange as well is that we adjust MEM_SIZE with bitpos, too. At least when the MEM has non-BLKmode this means that MEMs mode and MEM_SIZE is inconsistent? Or how do a MEMs mode and MEM_SIZE relate? I came up with the following patch that shouldn't change behavior but which makes it easier to follow what happens (it will also make bitpos handling with my alignment patch fix consistent). The patch moves attrs.align "precompute" into the !TYPE_P path and for the &DECL / &CONSTANT align compute use get_object_alignment which DTRT for it (it's still buggy because we later will use MAX of this alignment and the "correct" alignment - but I didn't want to change behavior here). The patch removes explicit align computation on the DECL_P and CONSTANT_CLASS_P paths because that's handled by the get_object_alignment path in the same way. That also allows the ARRAY_REF base paths to be simplified and commonized (the ARRAY_REF and then DECL_P path will get better align computation this way, get_object_alignment has more elaborate off_tree handling). As followup I'd remove the MEM_REF/TARGET_MEM_REF alignment "precompute" completely - it's buggy (due to the later use of MAX). That is, consider the below as explanatory intermediate step, any real patch would get rid of that "precompute" immediately (and then probably not re-use anything we got from default or pre-existing MEM_ATTRs but simply overwrite attrs.align. See 2nd patch below. Richard. 2013-01-10 Richard Biener <rguenther@suse.de> * emit-rtl.c (set_mem_attributes_minus_bitpos): Move MEM_REF handling completely into the !TYPE_P handling part and use get_object_alignment for its alignment computation. Always rely on get_object_alignment for final alignment computation. Index: gcc/emit-rtl.c =================================================================== *** gcc/emit-rtl.c (revision 195079) --- gcc/emit-rtl.c (working copy) *************** set_mem_attributes_minus_bitpos (rtx ref *** 1641,1691 **** if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type)) attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - else if (TREE_CODE (t) == MEM_REF) - { - tree op0 = TREE_OPERAND (t, 0); - if (TREE_CODE (op0) == ADDR_EXPR - && (DECL_P (TREE_OPERAND (op0, 0)) - || CONSTANT_CLASS_P (TREE_OPERAND (op0, 0)))) - { - if (DECL_P (TREE_OPERAND (op0, 0))) - attrs.align = DECL_ALIGN (TREE_OPERAND (op0, 0)); - else if (CONSTANT_CLASS_P (TREE_OPERAND (op0, 0))) - { - attrs.align = TYPE_ALIGN (TREE_TYPE (TREE_OPERAND (op0, 0))); - #ifdef CONSTANT_ALIGNMENT - attrs.align = CONSTANT_ALIGNMENT (TREE_OPERAND (op0, 0), - attrs.align); - #endif - } - if (TREE_INT_CST_LOW (TREE_OPERAND (t, 1)) != 0) - { - unsigned HOST_WIDE_INT ioff - = TREE_INT_CST_LOW (TREE_OPERAND (t, 1)); - unsigned HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT; - attrs.align = MIN (aoff, attrs.align); - } - } - else - /* ??? This isn't fully correct, we can't set the alignment from the - type in all cases. */ - attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - } - - else if (TREE_CODE (t) == TARGET_MEM_REF) - /* ??? This isn't fully correct, we can't set the alignment from the - type in all cases. */ - attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - /* If the size is known, we can set that. */ tree new_size = TYPE_SIZE_UNIT (type); /* If T is not a type, we may be able to deduce some more information about the expression. */ if (! TYPE_P (t)) { tree base; - bool align_computed = false; if (TREE_THIS_VOLATILE (t)) MEM_VOLATILE_P (ref) = 1; --- 1641,1657 ---- if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type)) attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); /* If the size is known, we can set that. */ tree new_size = TYPE_SIZE_UNIT (type); + /* The address-space is that of the type. */ + as = TYPE_ADDR_SPACE (type); + /* If T is not a type, we may be able to deduce some more information about the expression. */ if (! TYPE_P (t)) { tree base; if (TREE_THIS_VOLATILE (t)) MEM_VOLATILE_P (ref) = 1; *************** set_mem_attributes_minus_bitpos (rtx ref *** 1715,1720 **** --- 1681,1687 ---- && TREE_STATIC (base)) MEM_READONLY_P (ref) = 1; + /* Address-space information is on the base object. */ if (TREE_CODE (base) == MEM_REF || TREE_CODE (base) == TARGET_MEM_REF) as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base, *************** set_mem_attributes_minus_bitpos (rtx ref *** 1722,1729 **** else as = TYPE_ADDR_SPACE (TREE_TYPE (base)); } - else - as = TYPE_ADDR_SPACE (type); /* If this expression uses it's parent's alias set, mark it such that we won't change it. */ --- 1689,1694 ---- *************** set_mem_attributes_minus_bitpos (rtx ref *** 1738,1756 **** attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); - attrs.align = DECL_ALIGN (t); - align_computed = true; } ! /* If this is a constant, we know the alignment. */ else if (CONSTANT_CLASS_P (t)) ! { ! attrs.align = TYPE_ALIGN (type); ! #ifdef CONSTANT_ALIGNMENT ! attrs.align = CONSTANT_ALIGNMENT (t, attrs.align); ! #endif ! align_computed = true; ! } /* If this is a field reference, record it. */ else if (TREE_CODE (t) == COMPONENT_REF) --- 1703,1713 ---- attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); } ! /* ??? If we end up with a constant here do record a MEM_EXPR. */ else if (CONSTANT_CLASS_P (t)) ! ; /* If this is a field reference, record it. */ else if (TREE_CODE (t) == COMPONENT_REF) *************** set_mem_attributes_minus_bitpos (rtx ref *** 1795,1818 **** } while (TREE_CODE (t2) == ARRAY_REF); ! if (DECL_P (t2)) ! { ! attrs.expr = t2; ! attrs.offset_known_p = false; ! if (host_integerp (off_tree, 1)) ! { ! HOST_WIDE_INT ioff = tree_low_cst (off_tree, 1); ! HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT; ! attrs.align = DECL_ALIGN (t2); ! if (aoff && (unsigned HOST_WIDE_INT) aoff < attrs.align) ! attrs.align = aoff; ! align_computed = true; ! attrs.offset_known_p = true; ! attrs.offset = ioff; ! apply_bitpos = bitpos; ! } ! } ! else if (TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; --- 1752,1759 ---- } while (TREE_CODE (t2) == ARRAY_REF); ! if (DECL_P (t2) ! || TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; *************** set_mem_attributes_minus_bitpos (rtx ref *** 1822,1850 **** attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } - /* ??? Any reason the field size would be different than - the size we got from the type? */ } } /* If this is an indirect reference, record it. */ else if (TREE_CODE (t) == MEM_REF || TREE_CODE (t) == TARGET_MEM_REF) { attrs.expr = t; attrs.offset_known_p = true; attrs.offset = 0; apply_bitpos = bitpos; } ! if (!align_computed) ! { ! unsigned int obj_align = get_object_alignment (t); ! attrs.align = MAX (attrs.align, obj_align); ! } } - else - as = TYPE_ADDR_SPACE (type); if (host_integerp (new_size, 1)) { --- 1763,1813 ---- attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } } + /* Else do not record a MEM_EXPR. */ } /* If this is an indirect reference, record it. */ else if (TREE_CODE (t) == MEM_REF || TREE_CODE (t) == TARGET_MEM_REF) { + if (TREE_CODE (t) == MEM_REF) + { + tree op0 = TREE_OPERAND (t, 0); + if (TREE_CODE (op0) == ADDR_EXPR + && (DECL_P (TREE_OPERAND (op0, 0)) + || CONSTANT_CLASS_P (TREE_OPERAND (op0, 0)))) + { + attrs.align = get_object_alignment (TREE_OPERAND (op0, 0)); + if (TREE_INT_CST_LOW (TREE_OPERAND (t, 1)) != 0) + { + unsigned HOST_WIDE_INT ioff + = TREE_INT_CST_LOW (TREE_OPERAND (t, 1)); + unsigned HOST_WIDE_INT aoff + = (ioff & -ioff) * BITS_PER_UNIT; + attrs.align = MIN (aoff, attrs.align); + } + } + else + /* ??? This isn't fully correct, we can't set the alignment + from the type in all cases. */ + attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); + } + + else if (TREE_CODE (t) == TARGET_MEM_REF) + /* ??? This isn't fully correct, we can't set the alignment from the + type in all cases. */ + attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); + attrs.expr = t; attrs.offset_known_p = true; attrs.offset = 0; apply_bitpos = bitpos; } ! unsigned int obj_align = get_object_alignment (t); ! attrs.align = MAX (attrs.align, obj_align); } if (host_integerp (new_size, 1)) { 2013-01-10 Richard Biener <rguenther@suse.de> * emit-rtl.c (set_mem_attributes_minus_bitpos): Remove alignment computations and rely on get_object_alignment for the !TYPE_P case. Commonize DECL/COMPONENT_REF handling in the ARRAY_REF path. Index: gcc/emit-rtl.c =================================================================== *** gcc/emit-rtl.c (revision 195079) --- gcc/emit-rtl.c (working copy) *************** set_mem_attributes_minus_bitpos (rtx ref *** 1641,1691 **** if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type)) attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - else if (TREE_CODE (t) == MEM_REF) - { - tree op0 = TREE_OPERAND (t, 0); - if (TREE_CODE (op0) == ADDR_EXPR - && (DECL_P (TREE_OPERAND (op0, 0)) - || CONSTANT_CLASS_P (TREE_OPERAND (op0, 0)))) - { - if (DECL_P (TREE_OPERAND (op0, 0))) - attrs.align = DECL_ALIGN (TREE_OPERAND (op0, 0)); - else if (CONSTANT_CLASS_P (TREE_OPERAND (op0, 0))) - { - attrs.align = TYPE_ALIGN (TREE_TYPE (TREE_OPERAND (op0, 0))); - #ifdef CONSTANT_ALIGNMENT - attrs.align = CONSTANT_ALIGNMENT (TREE_OPERAND (op0, 0), - attrs.align); - #endif - } - if (TREE_INT_CST_LOW (TREE_OPERAND (t, 1)) != 0) - { - unsigned HOST_WIDE_INT ioff - = TREE_INT_CST_LOW (TREE_OPERAND (t, 1)); - unsigned HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT; - attrs.align = MIN (aoff, attrs.align); - } - } - else - /* ??? This isn't fully correct, we can't set the alignment from the - type in all cases. */ - attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - } - - else if (TREE_CODE (t) == TARGET_MEM_REF) - /* ??? This isn't fully correct, we can't set the alignment from the - type in all cases. */ - attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); - /* If the size is known, we can set that. */ tree new_size = TYPE_SIZE_UNIT (type); /* If T is not a type, we may be able to deduce some more information about the expression. */ if (! TYPE_P (t)) { tree base; - bool align_computed = false; if (TREE_THIS_VOLATILE (t)) MEM_VOLATILE_P (ref) = 1; --- 1641,1657 ---- if (objectp || TREE_CODE (t) == INDIRECT_REF || TYPE_ALIGN_OK (type)) attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); /* If the size is known, we can set that. */ tree new_size = TYPE_SIZE_UNIT (type); + /* The address-space is that of the type. */ + as = TYPE_ADDR_SPACE (type); + /* If T is not a type, we may be able to deduce some more information about the expression. */ if (! TYPE_P (t)) { tree base; if (TREE_THIS_VOLATILE (t)) MEM_VOLATILE_P (ref) = 1; *************** set_mem_attributes_minus_bitpos (rtx ref *** 1715,1720 **** --- 1681,1687 ---- && TREE_STATIC (base)) MEM_READONLY_P (ref) = 1; + /* Address-space information is on the base object. */ if (TREE_CODE (base) == MEM_REF || TREE_CODE (base) == TARGET_MEM_REF) as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base, *************** set_mem_attributes_minus_bitpos (rtx ref *** 1722,1729 **** else as = TYPE_ADDR_SPACE (TREE_TYPE (base)); } - else - as = TYPE_ADDR_SPACE (type); /* If this expression uses it's parent's alias set, mark it such that we won't change it. */ --- 1689,1694 ---- *************** set_mem_attributes_minus_bitpos (rtx ref *** 1738,1756 **** attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); - attrs.align = DECL_ALIGN (t); - align_computed = true; } ! /* If this is a constant, we know the alignment. */ else if (CONSTANT_CLASS_P (t)) ! { ! attrs.align = TYPE_ALIGN (type); ! #ifdef CONSTANT_ALIGNMENT ! attrs.align = CONSTANT_ALIGNMENT (t, attrs.align); ! #endif ! align_computed = true; ! } /* If this is a field reference, record it. */ else if (TREE_CODE (t) == COMPONENT_REF) --- 1703,1713 ---- attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); } ! /* ??? If we end up with a constant here do record a MEM_EXPR. */ else if (CONSTANT_CLASS_P (t)) ! ; /* If this is a field reference, record it. */ else if (TREE_CODE (t) == COMPONENT_REF) *************** set_mem_attributes_minus_bitpos (rtx ref *** 1795,1818 **** } while (TREE_CODE (t2) == ARRAY_REF); ! if (DECL_P (t2)) ! { ! attrs.expr = t2; ! attrs.offset_known_p = false; ! if (host_integerp (off_tree, 1)) ! { ! HOST_WIDE_INT ioff = tree_low_cst (off_tree, 1); ! HOST_WIDE_INT aoff = (ioff & -ioff) * BITS_PER_UNIT; ! attrs.align = DECL_ALIGN (t2); ! if (aoff && (unsigned HOST_WIDE_INT) aoff < attrs.align) ! attrs.align = aoff; ! align_computed = true; ! attrs.offset_known_p = true; ! attrs.offset = ioff; ! apply_bitpos = bitpos; ! } ! } ! else if (TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; --- 1752,1759 ---- } while (TREE_CODE (t2) == ARRAY_REF); ! if (DECL_P (t2) ! || TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; *************** set_mem_attributes_minus_bitpos (rtx ref *** 1822,1830 **** attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } - /* ??? Any reason the field size would be different than - the size we got from the type? */ } } /* If this is an indirect reference, record it. */ --- 1763,1770 ---- attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } } + /* Else do not record a MEM_EXPR. */ } /* If this is an indirect reference, record it. */ *************** set_mem_attributes_minus_bitpos (rtx ref *** 1837,1850 **** apply_bitpos = bitpos; } ! if (!align_computed) ! { ! unsigned int obj_align = get_object_alignment (t); ! attrs.align = MAX (attrs.align, obj_align); ! } } - else - as = TYPE_ADDR_SPACE (type); if (host_integerp (new_size, 1)) { --- 1777,1784 ---- apply_bitpos = bitpos; } ! attrs.align = get_object_alignment (t); } if (host_integerp (new_size, 1)) {
> Going over this what's strange as well is that we adjust MEM_SIZE > with bitpos, too. At least when the MEM has non-BLKmode this > means that MEMs mode and MEM_SIZE is inconsistent? Or how do > a MEMs mode and MEM_SIZE relate? Yes, the MEM attributes are incomplete/inconsistent between the call to set_mem_attributes_minus_bitpos and the subsequent adjustment by bitpos. That's why only the final result should matter and need be correct. Let me try to forge my own opinion on the PR before answering further.
Hi, On Thu, 10 Jan 2013, Eric Botcazou wrote: > > Going over this what's strange as well is that we adjust MEM_SIZE > > with bitpos, too. At least when the MEM has non-BLKmode this > > means that MEMs mode and MEM_SIZE is inconsistent? Or how do > > a MEMs mode and MEM_SIZE relate? > > Yes, the MEM attributes are incomplete/inconsistent between the call to > set_mem_attributes_minus_bitpos and the subsequent adjustment by bitpos. 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. 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. Ciao, Michael.
> 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. > 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).
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 <rguenther@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 <rguenther@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; + }
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 <rguenther@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 <rguenther@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; > + } >
> 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. What do we do for the 4.7 branch? The patch doesn't compile for it so I presume that there are one or more changes to be backported first?
Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c (revision 195014) +++ gcc/emit-rtl.c (working copy) @@ -1839,7 +1839,12 @@ set_mem_attributes_minus_bitpos (rtx ref if (!align_computed) { - unsigned int obj_align = get_object_alignment (t); + unsigned int obj_align; + unsigned HOST_WIDE_INT obj_bitpos; + get_object_alignment_1 (t, &obj_align, &obj_bitpos); + obj_bitpos = (obj_bitpos - apply_bitpos) & (obj_align - 1); + if (obj_bitpos != 0) + obj_align = (obj_bitpos & -obj_bitpos); attrs.align = MAX (attrs.align, obj_align); } }