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.
> >
> >> +}
> >> +
> >> /* 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.
> 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.
>
> >
> >> +
> >> /* 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).
> >
> >> 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
>
>
