> On May 29, 2024, at 02:57, Richard Biener <richard.guent...@gmail.com> wrote:
>
> On Tue, May 28, 2024 at 11:09 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>
>> Thank you for the comments. See my answers below:
>>
>> Joseph, please see the last question, I need your help on it. Thanks a lot
>> for the help.
>>
>> Qing
>>
>>> On May 28, 2024, at 03:38, Richard Biener <richard.guent...@gmail.com>
>>> wrote:
>>>
>>> On Fri, Apr 12, 2024 at 3:54 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>>>
>>>> Including the following changes:
>>>> * The definition of the new internal function .ACCESS_WITH_SIZE
>>>> in internal-fn.def.
>>>> * C FE converts every reference to a FAM with a "counted_by" attribute
>>>> to a call to the internal function .ACCESS_WITH_SIZE.
>>>> (build_component_ref in c_typeck.cc)
>>>>
>>>> This includes the case when the object is statically allocated and
>>>> initialized.
>>>> In order to make this working, the routines initializer_constant_valid_p_1
>>>> and output_constant in varasm.cc are updated to handle calls to
>>>> .ACCESS_WITH_SIZE.
>>>> (initializer_constant_valid_p_1 and output_constant in varasm.c)
>>>>
>>>> However, for the reference inside "offsetof", the "counted_by" attribute is
>>>> ignored since it's not useful at all.
>>>> (c_parser_postfix_expression in c/c-parser.cc)
>>>>
>>>> In addtion to "offsetof", for the reference inside operator "typeof" and
>>>> "alignof", we ignore counted_by attribute too.
>>>>
>>>> When building ADDR_EXPR for the .ACCESS_WITH_SIZE in C FE,
>>>> replace the call with its first argument.
>>>>
>>>> * Convert every call to .ACCESS_WITH_SIZE to its first argument.
>>>> (expand_ACCESS_WITH_SIZE in internal-fn.cc)
>>>> * Adjust alias analysis to exclude the new internal from clobbering
>>>> anything.
>>>> (ref_maybe_used_by_call_p_1 and call_may_clobber_ref_p_1 in
>>>> tree-ssa-alias.cc)
>>>> * Adjust dead code elimination to eliminate the call to .ACCESS_WITH_SIZE
>>>> when
>>>> it's LHS is eliminated as dead code.
>>>> (eliminate_unnecessary_stmts in tree-ssa-dce.cc)
>>>> * Provide the utility routines to check the call is .ACCESS_WITH_SIZE and
>>>> get the reference from the call to .ACCESS_WITH_SIZE.
>>>> (is_access_with_size_p and get_ref_from_access_with_size in tree.cc)
>>>>
>>>> gcc/c/ChangeLog:
>>>>
>>>> * c-parser.cc (c_parser_postfix_expression): Ignore the counted-by
>>>> attribute when build_component_ref inside offsetof operator.
>>>> * c-tree.h (build_component_ref): Add one more parameter.
>>>> * c-typeck.cc (build_counted_by_ref): New function.
>>>> (build_access_with_size_for_counted_by): New function.
>>>> (build_component_ref): Check the counted-by attribute and build
>>>> call to .ACCESS_WITH_SIZE.
>>>> (build_unary_op): When building ADDR_EXPR for
>>>> .ACCESS_WITH_SIZE, use its first argument.
>>>> (lvalue_p): Accept call to .ACCESS_WITH_SIZE.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> * internal-fn.cc (expand_ACCESS_WITH_SIZE): New function.
>>>> * internal-fn.def (ACCESS_WITH_SIZE): New internal function.
>>>> * tree-ssa-alias.cc (ref_maybe_used_by_call_p_1): Special case
>>>> IFN_ACCESS_WITH_SIZE.
>>>> (call_may_clobber_ref_p_1): Special case IFN_ACCESS_WITH_SIZE.
>>>> * tree-ssa-dce.cc (eliminate_unnecessary_stmts): Eliminate the call
>>>> to .ACCESS_WITH_SIZE when its LHS is dead.
>>>> * tree.cc (process_call_operands): Adjust side effect for function
>>>> .ACCESS_WITH_SIZE.
>>>> (is_access_with_size_p): New function.
>>>> (get_ref_from_access_with_size): New function.
>>>> * tree.h (is_access_with_size_p): New prototype.
>>>> (get_ref_from_access_with_size): New prototype.
>>>> * varasm.cc (initializer_constant_valid_p_1): Handle call to
>>>> .ACCESS_WITH_SIZE.
>>>> (output_constant): Handle call to .ACCESS_WITH_SIZE.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> * gcc.dg/flex-array-counted-by-2.c: New test.
>>>> ---
>>>> gcc/c/c-parser.cc | 10 +-
>>>> gcc/c/c-tree.h | 2 +-
>>>> gcc/c/c-typeck.cc | 128 +++++++++++++++++-
>>>> gcc/internal-fn.cc | 35 +++++
>>>> gcc/internal-fn.def | 4 +
>>>> .../gcc.dg/flex-array-counted-by-2.c | 112 +++++++++++++++
>>>> gcc/tree-ssa-alias.cc | 2 +
>>>> gcc/tree-ssa-dce.cc | 5 +-
>>>> gcc/tree.cc | 25 +++-
>>>> gcc/tree.h | 8 ++
>>>> gcc/varasm.cc | 10 ++
>>>> 11 files changed, 331 insertions(+), 10 deletions(-)
>>>> create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
>>>>
>>>> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
>>>> index c31349dae2ff..a6ed5ac43bb1 100644
>>>> --- a/gcc/c/c-parser.cc
>>>> +++ b/gcc/c/c-parser.cc
>>>> @@ -10850,9 +10850,12 @@ c_parser_postfix_expression (c_parser *parser)
>>>> if (c_parser_next_token_is (parser, CPP_NAME))
>>>> {
>>>> c_token *comp_tok = c_parser_peek_token (parser);
>>>> + /* Ignore the counted_by attribute for reference inside
>>>> + offsetof since the information is not useful at all. */
>>>> offsetof_ref
>>>> = build_component_ref (loc, offsetof_ref, comp_tok->value,
>>>> - comp_tok->location,
>>>> UNKNOWN_LOCATION);
>>>> + comp_tok->location,
>>>> UNKNOWN_LOCATION,
>>>> + false);
>>>> c_parser_consume_token (parser);
>>>> while (c_parser_next_token_is (parser, CPP_DOT)
>>>> || c_parser_next_token_is (parser,
>>>> @@ -10879,11 +10882,14 @@ c_parser_postfix_expression (c_parser *parser)
>>>> break;
>>>> }
>>>> c_token *comp_tok = c_parser_peek_token (parser);
>>>> + /* Ignore the counted_by attribute for reference
>>>> inside
>>>> + offsetof since the information is not useful.
>>>> */
>>>> offsetof_ref
>>>> = build_component_ref (loc, offsetof_ref,
>>>> comp_tok->value,
>>>> comp_tok->location,
>>>> - UNKNOWN_LOCATION);
>>>> + UNKNOWN_LOCATION,
>>>> + false);
>>>> c_parser_consume_token (parser);
>>>> }
>>>> else
>>>> diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
>>>> index 12fae8591462..402e8f78db2a 100644
>>>> --- a/gcc/c/c-tree.h
>>>> +++ b/gcc/c/c-tree.h
>>>> @@ -778,7 +778,7 @@ extern void mark_exp_read (tree);
>>>> extern tree composite_type (tree, tree);
>>>> extern tree lookup_field (const_tree, tree);
>>>> extern tree build_component_ref (location_t, tree, tree, location_t,
>>>> - location_t);
>>>> + location_t, bool = true);
>>>> extern tree build_array_ref (location_t, tree, tree);
>>>> extern tree build_omp_array_section (location_t, tree, tree, tree);
>>>> extern tree build_external_ref (location_t, tree, bool, tree *);
>>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
>>>> index fb7587f05f1f..ff6685c6c4ba 100644
>>>> --- a/gcc/c/c-typeck.cc
>>>> +++ b/gcc/c/c-typeck.cc
>>>> @@ -2578,15 +2578,116 @@ should_suggest_deref_p (tree datum_type)
>>>> return false;
>>>> }
>>>>
>>>> +/* For a SUBDATUM field of a structure or union DATUM, generate a REF to
>>>> + the object that represents its counted_by per the attribute counted_by
>>>> + attached to this field if it's a flexible array member field, otherwise
>>>> + return NULL_TREE.
>>>> + Set COUNTED_BY_TYPE to the TYPE of the counted_by field.
>>>> + For example, if:
>>>> +
>>>> + struct P {
>>>> + int k;
>>>> + int x[] __attribute__ ((counted_by (k)));
>>>> + } *p;
>>>> +
>>>> + for:
>>>> + p->x
>>>> +
>>>> + the ref to the object that represents its element count will be:
>>>> +
>>>> + &(p->k)
>>>> +
>>>> +*/
>>>> +static tree
>>>> +build_counted_by_ref (tree datum, tree subdatum, tree *counted_by_type)
>>>> +{
>>>> + tree type = TREE_TYPE (datum);
>>>> + if (!c_flexible_array_member_type_p (TREE_TYPE (subdatum)))
>>>> + return NULL_TREE;
>>>> +
>>>> + tree attr_counted_by = lookup_attribute ("counted_by",
>>>> + DECL_ATTRIBUTES (subdatum));
>>>> + tree counted_by_ref = NULL_TREE;
>>>> + *counted_by_type = NULL_TREE;
>>>> + if (attr_counted_by)
>>>> + {
>>>> + tree field_id = TREE_VALUE (TREE_VALUE (attr_counted_by));
>>>> + counted_by_ref
>>>> + = build_component_ref (UNKNOWN_LOCATION,
>>>> + datum, field_id,
>>>> + UNKNOWN_LOCATION, UNKNOWN_LOCATION);
>>>> + counted_by_ref = build_fold_addr_expr (counted_by_ref);
>>>> +
>>>> + /* Get the TYPE of the counted_by field. */
>>>> + tree counted_by_field = lookup_field (type, field_id);
>>>> + gcc_assert (counted_by_field);
>>>> +
>>>> + do
>>>> + {
>>>> + *counted_by_type = TREE_TYPE (TREE_VALUE (counted_by_field));
>>>> + counted_by_field = TREE_CHAIN (counted_by_field);
>>>> + }
>>>> + while (counted_by_field);
>>>> + }
>>>> + return counted_by_ref;
>>>> +}
>>>> +
>>>> +/* Given a COMPONENT_REF REF with the location LOC, the corresponding
>>>> + COUNTED_BY_REF, and the COUNTED_BY_TYPE, generate an INDIRECT_REF
>>>> + to a call to the internal function .ACCESS_WITH_SIZE.
>>>> +
>>>> + REF
>>>> +
>>>> + to:
>>>> +
>>>> + (*.ACCESS_WITH_SIZE (REF, COUNTED_BY_REF, 1, (TYPE_OF_SIZE)0, -1))
>>>> +
>>>> + NOTE: The return type of this function is the POINTER type pointing
>>>> + to the original flexible array type.
>>>> + Then the type of the INDIRECT_REF is the original flexible array type.
>>>> +
>>>> + The type of the first argument of this function is a POINTER type
>>>> + to the original flexible array type.
>>>> +
>>>> + The 4th argument of the call is a constant 0 with the TYPE of the
>>>> + object pointed by COUNTED_BY_REF.
>>>> +
>>>> + */
>>>> +static tree
>>>> +build_access_with_size_for_counted_by (location_t loc, tree ref,
>>>> + tree counted_by_ref,
>>>> + tree counted_by_type)
>>>> +{
>>>> + gcc_assert (c_flexible_array_member_type_p (TREE_TYPE (ref)));
>>>> + /* The result type of the call is a pointer to the flexible array type.
>>>> */
>>>> + tree result_type = build_pointer_type (TREE_TYPE (ref));
>>>> +
>>>> + tree call
>>>> + = build_call_expr_internal_loc (loc, IFN_ACCESS_WITH_SIZE,
>>>> + result_type, 5,
>>>> + array_to_pointer_conversion (loc, ref),
>>>> + counted_by_ref,
>>>> + build_int_cst (integer_type_node, 1),
>>>> + build_int_cst (counted_by_type, 0),
>>>> + build_int_cst (integer_type_node, -1));
>>>> + /* Wrap the call with an INDIRECT_REF with the flexible array type. */
>>>> + call = build1 (INDIRECT_REF, TREE_TYPE (ref), call);
>>>> + SET_EXPR_LOCATION (call, loc);
>>>> + return call;
>>>> +}
>>>> +
>>>> /* Make an expression to refer to the COMPONENT field of structure or
>>>> union value DATUM. COMPONENT is an IDENTIFIER_NODE. LOC is the
>>>> location of the COMPONENT_REF. COMPONENT_LOC is the location
>>>> of COMPONENT. ARROW_LOC is the location of the first -> operand if
>>>> - it is from -> operator. */
>>>> + it is from -> operator.
>>>> + If HANDLE_COUNTED_BY is true, check the counted_by attribute and
>>>> generate
>>>> + a call to .ACCESS_WITH_SIZE. Otherwise, ignore the attribute. */
>>>>
>>>> tree
>>>> build_component_ref (location_t loc, tree datum, tree component,
>>>> - location_t component_loc, location_t arrow_loc)
>>>> + location_t component_loc, location_t arrow_loc,
>>>> + bool handle_counted_by)
>>>> {
>>>> tree type = TREE_TYPE (datum);
>>>> enum tree_code code = TREE_CODE (type);
>>>> @@ -2658,7 +2759,13 @@ build_component_ref (location_t loc, tree datum,
>>>> tree component,
>>>> int quals;
>>>> tree subtype;
>>>> bool use_datum_quals;
>>>> -
>>>> + tree counted_by_type = NULL_TREE;
>>>> + /* Do not handle counted_by when in typeof and alignof operator.
>>>> */
>>>> + handle_counted_by = handle_counted_by && !in_typeof &&
>>>> !in_alignof;
>>>> + tree counted_by_ref = handle_counted_by
>>>> + ? build_counted_by_ref (datum, subdatum,
>>>> + &counted_by_type)
>>>> + : NULL_TREE;
>>>> if (TREE_TYPE (subdatum) == error_mark_node)
>>>> return error_mark_node;
>>>>
>>>> @@ -2677,6 +2784,12 @@ build_component_ref (location_t loc, tree datum,
>>>> tree component,
>>>> ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
>>>> NULL_TREE);
>>>> SET_EXPR_LOCATION (ref, loc);
>>>> +
>>>> + if (counted_by_ref)
>>>> + ref = build_access_with_size_for_counted_by (loc, ref,
>>>> + counted_by_ref,
>>>> + counted_by_type);
>>>> +
>>>> if (TREE_READONLY (subdatum)
>>>> || (use_datum_quals && TREE_READONLY (datum)))
>>>> TREE_READONLY (ref) = 1;
>>>> @@ -5080,7 +5193,11 @@ build_unary_op (location_t location, enum tree_code
>>>> code, tree xarg,
>>>> goto return_build_unary_op;
>>>> }
>>>>
>>>> - /* Ordinary case; arg is a COMPONENT_REF or a decl. */
>>>> + /* Ordinary case; arg is a COMPONENT_REF or a decl, or a call to
>>>> + .ACCESS_WITH_SIZE. */
>>>> + if (is_access_with_size_p (arg))
>>>> + arg = TREE_OPERAND (TREE_OPERAND (CALL_EXPR_ARG (arg, 0), 0), 0);
>>>> +
>>>> argtype = TREE_TYPE (arg);
>>>>
>>>> /* If the lvalue is const or volatile, merge that into the type
>>>> @@ -5227,6 +5344,9 @@ lvalue_p (const_tree ref)
>>>> case BIND_EXPR:
>>>> return TREE_CODE (TREE_TYPE (ref)) == ARRAY_TYPE;
>>>>
>>>> + case CALL_EXPR:
>>>> + return is_access_with_size_p (ref);
>>>> +
>>>> default:
>>>> return false;
>>>> }
>>>> diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc
>>>> index a07f25f3aee3..e744080ee670 100644
>>>> --- a/gcc/internal-fn.cc
>>>> +++ b/gcc/internal-fn.cc
>>>> @@ -3393,6 +3393,41 @@ expand_DEFERRED_INIT (internal_fn, gcall *stmt)
>>>> }
>>>> }
>>>>
>>>> +/* Expand the IFN_ACCESS_WITH_SIZE function:
>>>> + ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE,
>>>> + TYPE_OF_SIZE, ACCESS_MODE)
>>>> + which returns the REF_TO_OBJ same as the 1st argument;
>>>> +
>>>> + 1st argument REF_TO_OBJ: The reference to the object;
>>>> + 2nd argument REF_TO_SIZE: The reference to the size of the object,
>>>> + 3rd argument CLASS_OF_SIZE: The size referenced by the REF_TO_SIZE
>>>> represents
>>>> + 0: the number of bytes.
>>>> + 1: the number of the elements of the object type;
>>>> + 4th argument TYPE_OF_SIZE: A constant 0 with its TYPE being the same
>>>> as the TYPE
>>>> + of the object referenced by REF_TO_SIZE
>>>> + 5th argument ACCESS_MODE:
>>>> + -1: Unknown access semantics
>>>> + 0: none
>>>> + 1: read_only
>>>> + 2: write_only
>>>> + 3: read_write
>>>> +
>>>> + Both the return type and the type of the first argument of this
>>>> + function have been converted from the incomplete array type to
>>>> + the corresponding pointer type.
>>>> +
>>>> + For each call to a .ACCESS_WITH_SIZE, replace it with its 1st
>>>> argument. */
>>>> +static void
>>>> +expand_ACCESS_WITH_SIZE (internal_fn, gcall *stmt)
>>>> +{
>>>> + tree lhs = gimple_call_lhs (stmt);
>>>> + tree ref_to_obj = gimple_call_arg (stmt, 0);
>>>> + if (lhs)
>>>> + expand_assignment (lhs, ref_to_obj, false);
>>>> + else
>>>> + emit_insn (expand_normal (ref_to_obj));
>>>
>>> That looks suspicious, expand_normal does not result in an insn. What is
>>> there to do when there's no LHS of the .ACCESS_WITH_SIZE?
>>
>> I think my initial purpose of that line is trying to handle such rare
>> situation that might happen with different optimizations..
>> I can change the else path to a gcc_unreachable() to see any issue.
>> What do you think?
>
> As I've seen later in DCE you happily elide .ACCESS_WITH_SIZE without
> a LHS so I'd
> say you should simply do nothing (remove the else) here which means
> DCEing the call.
Okay, will do that. Thanks.
>
>>>
>>>> +}
>>>> +
>>>> /* The size of an OpenACC compute dimension. */
>>>>
>>>> static void
>>>> diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
>>>> index c14d30365c14..0801c8bfe61d 100644
>>>> --- a/gcc/internal-fn.def
>>>> +++ b/gcc/internal-fn.def
>>>> @@ -510,6 +510,10 @@ DEF_INTERNAL_FN (PHI, 0, NULL)
>>>> automatic variable. */
>>>> DEF_INTERNAL_FN (DEFERRED_INIT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
>>>>
>>>> +/* A function to associate the access size and access mode information
>>>> + with the corresponding reference to an object. */
>>>> +DEF_INTERNAL_FN (ACCESS_WITH_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)
>>>
>>> Does .ACCESS_WITH_SIZE store to memory? (or even load?)
>> It reads from the 2nd argument.
>> So, it’s not PURE.
>
> If it only reads it _is_ PURE. It's not CONST.
Just read the documentation of GCC pure attribute:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute
So, ECF_PURE has the same meaning of the pure attribute?
If so, yes, .ACCESS_WITH_SIZE is ECF_PURE.
Thanks a lot for catching this bug.
Will update it.
>
>> Please see our discussion last November on this:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635158.html
>>
>> In which: "Although .ACCESS_WITH_SIZE is not PURE anymore, but it’s only
>> read from the 2nd argument, and not modify anything in the pointed objects.
>> So, we can adjust the IPA alias analysis phase with this details
>> (ref_maybe_used_by_call_p_1/call_may_clobber_ref_p_1).”
>>
>> (NOTE, in the initial proposal
>> (https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634844.html) I
>> defined it as
>> DEF_INTERNAL_FN (ACCESS_WITH_SIZE, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL),
>> And later after discussion, we deleted ECF_CONST from it)
>
> And you now should put in ECF_PURE.
Okay.
>
>>
>>>
>>>> +
>>>> /* DIM_SIZE and DIM_POS return the size of a particular compute
>>>> dimension and the executing thread's position within that
>>>> dimension. DIM_POS is pure (and not const) so that it isn't
>>>> diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
>>>> b/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
>>>> new file mode 100644
>>>> index 000000000000..d4899a63af3c
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
>>>> @@ -0,0 +1,112 @@
>>>> +/* Test the code generation for the new attribute counted_by.
>>>> + And also the offsetof operator on such array. */
>>>> +/* { dg-do run } */
>>>> +/* { dg-options "-O2 -fdump-tree-original" } */
>>>> +
>>>> +#include <stdlib.h>
>>>> +
>>>> +struct annotated {
>>>> + int b;
>>>> + char c[] __attribute__ ((counted_by (b)));
>>>> +} *array_annotated;
>>>> +
>>>> +static struct annotated static_annotated = { sizeof "hello", "hello" };
>>>> +static char *y = static_annotated.c;
>>>> +
>>>> +struct flex {
>>>> + int b;
>>>> + char c[];
>>>> +};
>>>> +
>>>> +struct nested_annotated {
>>>> + struct {
>>>> + union {
>>>> + int b;
>>>> + float f;
>>>> + };
>>>> + int n;
>>>> + };
>>>> + char c[] __attribute__ ((counted_by (b)));
>>>> +} *array_nested_annotated;
>>>> +
>>>> +static struct nested_annotated nested_static_annotated
>>>> + = { sizeof "hello1", 0, "hello1" };
>>>> +static char *nested_y = nested_static_annotated.c;
>>>> +
>>>> +struct nested_flex {
>>>> + struct {
>>>> + union {
>>>> + int b;
>>>> + float f;
>>>> + };
>>>> + int n;
>>>> + };
>>>> + char c[];
>>>> +};
>>>> +
>>>> +void __attribute__((__noinline__)) setup (int normal_count, int
>>>> attr_count)
>>>> +{
>>>> + array_annotated
>>>> + = (struct annotated *)malloc (sizeof (struct annotated)
>>>> + + attr_count * sizeof (char));
>>>> + array_annotated->b = attr_count;
>>>> +
>>>> + array_nested_annotated
>>>> + = (struct nested_annotated *)malloc (sizeof (struct nested_annotated)
>>>> + + attr_count * sizeof (char));
>>>> + array_nested_annotated->b = attr_count;
>>>> +
>>>> + return;
>>>> +}
>>>> +
>>>> +void __attribute__((__noinline__)) test (char a, char b)
>>>> +{
>>>> + if (__builtin_offsetof (struct annotated, c[0])
>>>> + != __builtin_offsetof (struct flex, c[0]))
>>>> + abort ();
>>>> + if (__builtin_offsetof (struct annotated, c[1])
>>>> + != __builtin_offsetof (struct flex, c[1]))
>>>> + abort ();
>>>> + if (__builtin_offsetof (struct nested_annotated, c[0])
>>>> + != __builtin_offsetof (struct nested_flex, c[0]))
>>>> + abort ();
>>>> + if (__builtin_offsetof (struct nested_annotated, c[1])
>>>> + != __builtin_offsetof (struct nested_flex, c[1]))
>>>> + abort ();
>>>> +
>>>> + if (__builtin_types_compatible_p (typeof (array_annotated->c),
>>>> + typeof (&(array_annotated->c)[0])))
>>>> + abort ();
>>>> + if (__builtin_types_compatible_p (typeof (array_nested_annotated->c),
>>>> + typeof
>>>> (&(array_nested_annotated->c)[0])))
>>>> + abort ();
>>>> +
>>>> + if (__alignof (array_annotated->c) != __alignof (char))
>>>> + abort ();
>>>> + if (__alignof (array_nested_annotated->c) != __alignof (char))
>>>> + abort ();
>>>> +
>>>> + if ((unsigned long) array_annotated->c != (unsigned long)
>>>> &array_annotated->c)
>>>> + abort ();
>>>> + if ((unsigned long) array_nested_annotated->c
>>>> + != (unsigned long) &array_nested_annotated->c)
>>>> + abort ();
>>>> +
>>>> + array_annotated->c[2] = a;
>>>> + array_nested_annotated->c[3] = b;
>>>> +
>>>> + if (y[2] != 'l') abort ();
>>>> + if (nested_y[4] !='o') abort ();
>>>> +
>>>> +}
>>>> +
>>>> +int main(int argc, char *argv[])
>>>> +{
>>>> + setup (10,10);
>>>> + test ('A', 'B');
>>>> + if (array_annotated->c[2] != 'A') abort ();
>>>> + if (array_nested_annotated->c[3] != 'B') abort ();
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/* { dg-final { scan-tree-dump-times "ACCESS_WITH_SIZE" 8 "original" } }
>>>> */
>>>> diff --git a/gcc/tree-ssa-alias.cc b/gcc/tree-ssa-alias.cc
>>>> index e7c1c1aa6243..8c070e173bdd 100644
>>>> --- a/gcc/tree-ssa-alias.cc
>>>> +++ b/gcc/tree-ssa-alias.cc
>>>> @@ -2823,6 +2823,7 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref
>>>> *ref, bool tbaa_p)
>>>> return false;
>>>> case IFN_MASK_STORE_LANES:
>>>> case IFN_MASK_LEN_STORE_LANES:
>>>> + case IFN_ACCESS_WITH_SIZE:
>>>
>>> below you make it not store, so grouping with store IFNs is a bit odd?
>>> What's wrong with
>>> going through default: for IFN_ACCESS_WITH_SIZE? Does .ACCESS_WITH_SIZE
>>> perform a read? How's that represented?
>> Yes, it reads from the 2nd argument when used by
>> __builtin_dynamic_object_size, or c-ubsan, etc.
>>
>> .ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE, SIZE_OF_SIZE,
>> ACCESS_MODE, INDEX)
>>
>> The 2nd argument is the reference to the size of the object (which is the
>> field that holds the “counted-by” value in the structure).
>> When __builtin_dynamic_object_size queries the size of the object, GCC will
>> generate a MEM_REF to the 2nd argument to get the size of the object, please
>> see the routine “access_with_size_object_size” in gcc/tree-object-size.cc
>> <http://tree-object-size.cc/>.
>
> But "process_args" doesn't consider that - that only considers aggregate uses.
> Your change handles the call like if it were CONST. The default handling
> should
> be fine here, you shouldn't need to handle .ACCESS_WITH_SIZE in
> tree-ssa-alias.cc at all
> (if you make it PURE).
Yes, marking it as PURE will make the implementation simpler.
Thanks a lot.
Will update this.
Qing
>
>>>
>>>> goto process_a
>>>> case IFN_MASK_LOAD:
>>>> case IFN_LEN_LOAD:
>>>> @@ -3073,6 +3074,7 @@ call_may_clobber_ref_p_1 (gcall *call, ao_ref *ref,
>>>> bool tbaa_p)
>>>> case IFN_UBSAN_OBJECT_SIZE:
>>>> case IFN_UBSAN_PTR:
>>>> case IFN_ASAN_CHECK:
>>>> + case IFN_ACCESS_WITH_SIZE:
>>>
>>> it doesn't store. So above it should be ECF_PURE at least.
>>
>> As summarized in
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635158.html
>>
>> ACCESS_WITH_SIZE is not PURE anymore, but it’s only read from the 2nd
>> argument, and not modify anything in the pointed objects.
>
> I think it is.
>
>> Therefore, I think the change in the above is correct.
>>
>> i.e:
>> /* Treat these internal calls like ECF_PURE for aliasing,
>> they don't write to any memory the program should care about.
>> They have important other side-effects, and read memory,
>> so can't be ECF_NOVOPS. */
>> case IFN_UBSAN_NULL:
>> case IFN_UBSAN_BOUNDS:
>> case IFN_UBSAN_VPTR:
>> case IFN_UBSAN_OBJECT_SIZE:
>> case IFN_UBSAN_PTR:
>> case IFN_ASAN_CHECK:
>> case IFN_ACCESS_WITH_SIZE:
>> return false;
>>
>> What do you think?
>>
>>
>>>> return false;
>>>> case IFN_MASK_STORE:
>>>> case IFN_LEN_STORE:
>>>> diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc
>>>> index 636c471d4c89..a54fb1b754dd 100644
>>>> --- a/gcc/tree-ssa-dce.cc
>>>> +++ b/gcc/tree-ssa-dce.cc
>>>> @@ -1459,8 +1459,8 @@ eliminate_unnecessary_stmts (bool aggressive)
>>>> update_stmt (stmt);
>>>> release_ssa_name (name);
>>>>
>>>> - /* GOMP_SIMD_LANE (unless three argument) or ASAN_POISON
>>>> - without lhs is not needed. */
>>>> + /* GOMP_SIMD_LANE (unless three argument), ASAN_POISON
>>>> + or .ACCESS_WITH_SIZE without lhs is not needed. */
>>>> if (gimple_call_internal_p (stmt))
>>>> switch (gimple_call_internal_fn (stmt))
>>>> {
>>>> @@ -1470,6 +1470,7 @@ eliminate_unnecessary_stmts (bool aggressive)
>>>> break;
>>>> /* FALLTHRU */
>>>> case IFN_ASAN_POISON:
>>>> + case IFN_ACCESS_WITH_SIZE:
>>>
>>> this shouldn't be necessary if you make .ACCESS_WITH_SIZE ECF_PURE
>>> (or ECF_CONST if it also doesn't read from memory)
>> As explained in the above, .ACCESS_WITH_SIZE is not PURE.
>>>
>>>> remove_dead_stmt (&gsi, bb, to_remove_edges);
>>>> break;
>>>> default:
>>>> diff --git a/gcc/tree.cc b/gcc/tree.cc
>>>> index 3dff8c510832..5fdb425f612a 100644
>>>> --- a/gcc/tree.cc
>>>> +++ b/gcc/tree.cc
>>>> @@ -4068,7 +4068,8 @@ process_call_operands (tree t)
>>>> int i = call_expr_flags (t);
>>>>
>>>> /* Calls have side-effects, except those to const or pure functions. */
>>>> - if ((i & ECF_LOOPING_CONST_OR_PURE) || !(i & (ECF_CONST | ECF_PURE)))
>>>> + if ((i & ECF_LOOPING_CONST_OR_PURE)
>>>> + || (!(i & (ECF_CONST | ECF_PURE)) && !is_access_with_size_p (t)))
>>>
>>> Err, so why not make it ECF_PURE at least?
>> Same above.
>>>
>>>> side_effects = true;
>>>> /* Propagate TREE_READONLY of arguments for const functions. */
>>>> if (i & ECF_CONST)
>>>> @@ -13362,6 +13363,28 @@ component_ref_size (tree ref,
>>>> special_array_member *sam /* = NULL */)
>>>> ? NULL_TREE : size_zero_node);
>>>> }
>>>>
>>>> +/* Return true if the given node CALL is a call to a .ACCESS_WITH_SIZE
>>>> + function. */
>>>> +bool
>>>> +is_access_with_size_p (const_tree call)
>>>> +{
>>>> + if (TREE_CODE (call) != CALL_EXPR)
>>>> + return false;
>>>> + if (CALL_EXPR_IFN (call) == IFN_ACCESS_WITH_SIZE)
>>>> + return true;
>>>> + return false;
>>>> +}
>>>> +
>>>> +/* Get the corresponding reference from the call to a .ACCESS_WITH_SIZE.
>>>> + * i.e the first argument of this call. Return NULL_TREE otherwise. */
>>>> +tree
>>>> +get_ref_from_access_with_size (tree call)
>>>> +{
>>>> + if (is_access_with_size_p (call))
>>>> + return CALL_EXPR_ARG (call, 0);
>>>> + return NULL_TREE;
>>>> +}
>>>> +
>>>> /* Return the machine mode of T. For vectors, returns the mode of the
>>>> inner type. The main use case is to feed the result to HONOR_NANS,
>>>> avoiding the BLKmode that a direct TYPE_MODE (T) might return. */
>>>> diff --git a/gcc/tree.h b/gcc/tree.h
>>>> index 972a067a1f7a..fbaef3e5fb5c 100644
>>>> --- a/gcc/tree.h
>>>> +++ b/gcc/tree.h
>>>> @@ -5760,6 +5760,14 @@ extern special_array_member component_ref_sam_type
>>>> (tree);
>>>> cannot be determined. */
>>>> extern tree component_ref_size (tree, special_array_member * = NULL);
>>>>
>>>> +/* Return true if the given node is a call to a .ACCESS_WITH_SIZE
>>>> + function. */
>>>> +extern bool is_access_with_size_p (const_tree);
>>>> +
>>>> +/* Get the corresponding reference from the call to a .ACCESS_WITH_SIZE,
>>>> + * i.e. the first argument of this call. Return NULL_TREE otherwise. */
>>>> +extern tree get_ref_from_access_with_size (tree);
>>>> +
>>>> extern int tree_map_base_eq (const void *, const void *);
>>>> extern unsigned int tree_map_base_hash (const void *);
>>>> extern bool tree_map_base_marked_p (const void *);
>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
>>>> index fa17eff551e8..d75b23668925 100644
>>>> --- a/gcc/varasm.cc
>>>> +++ b/gcc/varasm.cc
>>>> @@ -5082,6 +5082,11 @@ initializer_constant_valid_p_1 (tree value, tree
>>>> endtype, tree *cache)
>>>> }
>>>> return ret;
>>>>
>>>> + case CALL_EXPR:
>>>> + /* For a call to .ACCESS_WITH_SIZE, check the first argument. */
>>>> + if (tree ref = get_ref_from_access_with_size (value))
>>>> + return initializer_constant_valid_p_1 (ref, endtype, cache);
>>>
>>> I think we should fold/strip .ACCESS_WITH_SIZE from initializers
>>> instead. That would be
>>> the frontends job I guess, most probably not even generate those in
>>> the first place?
>>
>> Sounds reasonable, I will see how to do this in C FE.
>> Joseph, do you have any suggestion where in C FE I should do this folding?
>>
>> thanks.
>>
>> Qing
>>>
>>>> + /* FALLTHROUGH. */
>>>> default:
>>>> break;
>>>> }
>>>> @@ -5276,6 +5281,11 @@ output_constant (tree exp, unsigned HOST_WIDE_INT
>>>> size, unsigned int align,
>>>> exp = TREE_OPERAND (exp, 0);
>>>> }
>>>>
>>>> + /* For a call to .ACCESS_WITH_SIZE, check the first argument. */
>>>> + if (TREE_CODE (exp) == CALL_EXPR)
>>>> + if (tree ref = get_ref_from_access_with_size (exp))
>>>> + exp = ref;
>>>> +
>>>> code = TREE_CODE (TREE_TYPE (exp));
>>>> thissize = int_size_in_bytes (TREE_TYPE (exp));
>>>>
>>>> --
>>>> 2.31.1
