On Thu, 3 Feb 2022, Richard Sandiford wrote:
> Richard Biener <rguent...@suse.de> writes:
> > On Thu, 3 Feb 2022, Richard Sandiford wrote:
> >
> >> Richard Biener <rguent...@suse.de> writes:
> >> > On Wed, 2 Feb 2022, Michael Matz wrote:
> >> >
> >> >> Hello,
> >> >>
> >> >> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
> >> >>
> >> >> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers
> >> >> > this
> >> >> > marks the end-of-life of storage as opposed to just ending the
> >> >> > lifetime
> >> >> > of the object that occupied it. The dangling pointer diagnostics uses
> >> >> > CLOBBERs but is confused by those emitted by the C++ frontend for
> >> >> > example which emits them for the second purpose at the start of
> >> >> > CTORs.
> >> >> > The issue is also appearant for aarch64 in PR104092.
> >> >> >
> >> >> > Distinguishing the two cases is also necessary for the PR90348 fix.
> >> >>
> >> >> (Just FWIW: I agree with the plan to have different types of CLOBBERs,
> >> >> in
> >> >> particular those for marking births)
> >> >>
> >> >> A style nit:
> >> >>
> >> >> > tree clobber = build_clobber (TREE_TYPE (t));
> >> >> > + CLOBBER_MARKS_EOL (clobber) = 1;
> >> >>
> >> >> I think it really were better if build_clobber() gained an additional
> >> >> argument (non-optional!) that sets the type of it.
> >> >
> >> > It indeed did occur to me as well, so as we're two now I tried to see
> >> > how it looks like. It does like the following (didn't bother to
> >> > replace all build_clobber calls but defaulted the parameter - there
> >> > are too many). With CLOBBER_BIRTH added for the fix for PR90348
> >> > the enum would be constructed like
> >> >
> >> > #define CLOBBER_KIND(NODE) \
> >> > ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
> >> > | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))
> >> >
> >> > or so (maybe different dependent on bitfield endianess to make
> >> > extracting the two adjacent bits cheap). That would leave us space
> >> > for a fourth state.
> >> >
> >> > So do you think that makes it nicer? What do others think?
> >>
> >> This way looks cleaner to me FWIW.
> >>
> >> Which arm of tree_base::u do constructors use? If we need a 2-bit
> >> enum then maybe we can carve one out from there.
> >
> > constructors are tcc_exceptional but since they are used where
> > tree operands reside they need to be compatible to EXPR_P and
> > thus various things like TREE_SIDE_EFFECTS need to work.
> >
> > Which means, we can't.
>
> But TREE_SIDE_EFFECTS is part of the main set of flags. For tree_base::u
> we have:
>
> union {
> /* The bits in the following structure should only be used with
> accessor macros that constrain inputs with tree checking. */
> struct {
> unsigned lang_flag_0 : 1;
> unsigned lang_flag_1 : 1;
> unsigned lang_flag_2 : 1;
> unsigned lang_flag_3 : 1;
> unsigned lang_flag_4 : 1;
> unsigned lang_flag_5 : 1;
> unsigned lang_flag_6 : 1;
> unsigned saturating_flag : 1;
>
> unsigned unsigned_flag : 1;
> unsigned packed_flag : 1;
> unsigned user_align : 1;
> unsigned nameless_flag : 1;
> unsigned atomic_flag : 1;
> unsigned unavailable_flag : 1;
> unsigned spare0 : 2;
>
> unsigned spare1 : 8;
>
> /* This field is only used with TREE_TYPE nodes; the only reason it is
> present in tree_base instead of tree_type is to save space. The size
> of the field must be large enough to hold addr_space_t values. */
> unsigned address_space : 8;
> } bits;
>
> /* The following fields are present in tree_base to save space. The
> nodes using them do not require any of the flags above and so can
> make better use of the 4-byte sized word. */
>
> /* The number of HOST_WIDE_INTs in an INTEGER_CST. */
> struct {
> /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
> its native precision. */
> unsigned char unextended;
>
> /* The number of HOST_WIDE_INTs if the INTEGER_CST is extended to
> wider precisions based on its TYPE_SIGN. */
> unsigned char extended;
>
> /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
> offset_int precision, with smaller integers being extended
> according to their TYPE_SIGN. This is equal to one of the two
> fields above but is cached for speed. */
> unsigned char offset;
> } int_length;
>
> /* VEC length. This field is only used with TREE_VEC. */
> int length;
>
> /* This field is only used with VECTOR_CST. */
> struct {
> /* The value of VECTOR_CST_LOG2_NPATTERNS. */
> unsigned int log2_npatterns : 8;
>
> /* The value of VECTOR_CST_NELTS_PER_PATTERN. */
> unsigned int nelts_per_pattern : 8;
>
> /* For future expansion. */
> unsigned int unused : 16;
> } vector_cst;
>
> /* SSA version number. This field is only used with SSA_NAME. */
> unsigned int version;
>
> /* CHREC_VARIABLE. This field is only used with POLYNOMIAL_CHREC. */
> unsigned int chrec_var;
>
> /* Internal function code. */
> enum internal_fn ifn;
>
> /* OMP_ATOMIC* memory order. */
> enum omp_memory_order omp_atomic_memory_order;
>
> /* The following two fields are used for MEM_REF and TARGET_MEM_REF
> expression trees and specify known data non-dependences. For
> two memory references in a function they are known to not
> alias if dependence_info.clique are equal and dependence_info.base
> are distinct. Clique number zero means there is no information,
> clique number one is populated from function global information
> and thus needs no remapping on transforms like loop unrolling. */
> struct {
> unsigned short clique;
> unsigned short base;
> } dependence_info;
> } GTY((skip(""))) u;
>
> so there's spare room even if constructors use the general “bits” arm.
Ah, yes - I thought CTORs are using that for the number of elements
but they don't, they use a pointer to a vec<>. So yes, for convenience
we could use bits in that section (with a bit more churn as they need to
be streamed for LTO, etc.)
I'll see to simply stick the plain enum there for CTORs.
Richard.
