On Thu, Nov 30, 2023 at 5:13 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Nov 29, 2023, Richard Biener <richard.guent...@gmail.com> wrote:
>
> >> Because &arg_#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.
>
> > 'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
> > so I assume it was &MEM[arg_(D)][n_#] which is indeed OK.
>
> Yeah.
>
> > But you shouldn't need to change a pointer argument to be passed by
> > reference, do you?
>
> True, my attempt to simplify the example moved it past the breaking point.
>
> IIRC the actual situations I hit involved computing address of members
> of compound objects, such as struct members, even array elements
> thereof. They became problematic after replacing the object with a
> dereference in gimple stmts. The (effectively) offsetting operation is
> well-formed gimple, but IIRC adding dereferencing to it made it
> malformed gimple. I don't immediately see why this should be the case,
> since it's still offsetting, so perhaps I misremember.
>
> > As said, you want to restrict by-reference passing to arguments
> > that are !is_gimple_reg_type ()
>
> *nod*, it was already there:
>
> if (!(0 /* DECL_BY_REFERENCE (narg) */
> || is_gimple_reg_type (TREE_TYPE (nparm))
> ...
> {
> indirect_nparms.add (nparm);
>
> >> Here are changes.html entries for this and for the other newly-added
> >> features:
>
> > LGTM.
>
> Was that an ok to install, once the relevant pieces are in?
See below.
> > Can you check on the pass-by-reference thing again please?
>
> Sure. I'll get back to you shortly.
>
> If argument indirection becomes the only blocking issue, I'd be happy to
> disable it, or even split out the patch that introduces it, so that the
> bulk of the feature can go in while we sort out these details.
> Disabling it is as simple as:
>
> diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc
> index 293bec132b885..90770202fb851 100644
> --- a/gcc/ipa-strub.cc
> +++ b/gcc/ipa-strub.cc
> @@ -2831,6 +2831,7 @@ pass_ipa_strub::execute (function *)
> parm = DECL_CHAIN (parm),
> nparm = DECL_CHAIN (nparm),
> nparmt = nparmt ? TREE_CHAIN (nparmt) : NULL_TREE)
> + if (true) ; else // ??? Disable parm indirection for now.
> if (!(0 /* DECL_BY_REFERENCE (narg) */
> || is_gimple_reg_type (TREE_TYPE (nparm))
> || VECTOR_TYPE_P (TREE_TYPE (nparm))
>
>
> > Let's see if Honza or Martin have any comments on the IPA bits, I just
> > mentioned what I think should be doable ...
>
> I'm curious as to what you're hoping for. I mean, I am using
> create_version_clone_with_body, adding the new params and copying the
> preexisting ones, and modifying some argument types for indirection
> after cloning. The problems I faced were as I tried to replace params
> with their indirected versions. According to my notes and my
> recollection, that's where I hit most of the trouble. But what would
> this really buy us? Do you envision a possibility of actually splitting
> out the original function body, so that IPA takes care of the whole
> wrapping? AFAICT that would require a lot more infrastructure to deal
> with new and modified parameters, though the details of what I learned
> about this API back then, and that made it clear I wouldn't be able to
> use it, seem to have faded away from my memory.
In the end I was hoping for general comments on the cgraph usage
and for the specifics indeed being able to use IPA mechanisms
to perform the stmt re-writing (and re-gimplification) for the value to
by-reference replacement. The core copy_body mechanism
might support it by simply registering *new_param as replacement
for 'old_param' in the copy_body_data decl_map.
Iff the IPA folks (Honza or Martin) don't have any further comments
the patch is OK to install by next Monday.
Thanks,
Richard.
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
