Re: [Patch] Fortran: Avoid SAVE_EXPR for deferred-len char types

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Feb 20, 2023 at 5:23 PM Tobias Burnus <tobias@codesourcery.com> wrote:
>
> Hi Richard, hi all,
>
> On 20.02.23 13:46, Richard Biener wrote:
> > +      /* TODO: A more middle-end friendly alternative would be to use NULL_TREE
> > +        as upper bound and store the value, e.g. as GFC_DECL_STRING_LEN.
> > +        Caveat: this requires some cleanup throughout the code to consistently
> > +        use some wrapper function.  */
> > +      gcc_assert (TREE_CODE (TYPE_SIZE_UNIT (type)) == SAVE_EXPR);
> > +      tree tmp = TREE_TYPE (TYPE_SIZE (eltype));
> >
> > ...
> >
> > you are probably breaking type sharing here.  You could use
> > build_array_type_1 and pass false for 'shared' to get around that.  Note
> > that there's also canonical type building done in case 'eltype' is not
> > canonical itself.
>
> My feeling is that this is already somewhat broken. Currently, there
> is one type per decl as each has its own artificial length variable.
> I have no idea how this will be handled in the ME in terms of alias
> analysis. And whether shared=false makes sense here and what effect
> is has. (Probably yes.)
>
> In principle,
>    integer(kind=8) .str., .str2;
>    character(kind=1)[1:.str] * str;
>    character(kind=1)[1:.str2] * str2;
> have the same type and iff .str == .str at runtime, they can alias.
> Example:
>    str2 = str;
>    .str2 = .str;
>
> I have no idea how the type analysis currently works (with or without SAVE_EXPR)
> nor what effect shared=false has in this case.

alias analysis for array types looks only at the element type

> > The solution to the actual problem is a hack - you are relying on
> > re-evaluation of TYPE_SIZE, and for that, only from within accesses
> > from inside the frontend?
>
> I think this mostly helps with access inside the FE of the type 'size =
> TYPE_SIZE_UNIT(type)', which is used surprisingly often and is often
> directly evaluated (i.e. assigned to a temporary).

that's what I thought

> > Since gimplification will produce the result into a single temporary again, re-storing the "breakage".
> > So, does it_really_  fix things?
>
> It does seem to fix cases which do  'size = TYPE_SIZE_UNIT (type);' in
> the front end and then uses this size expression. Thus, there are fixed.
> However, there are many cases where things go wrong - with and without
> the patch. I keep discovering more and more :-(

I guess test coverage isn't too great with this feature then ;)

> * * *
>
> I still think that the proper way is to have NULL_TREE as upper value
> would be better in several ways, except that there is (too) much code

Yep.

> which relies on TYPE_UNIT_SIZE to work. (There are 117 occurrences).
> Additionally, there is more code doing assumptions in this area.
>
> Thus, the question is whether it makes sense as hackish partial solution
> or whether it should remain in the current broken stage until it is
> fixed properly.

I wonder if it makes more sense to individually fix the places using
TYPE_UNIT_SIZE in a wrong way?  You'd also get only "partial"
fixes, but at least those will be true and good?

Otherwise I defer to frontend maintainers if they agree to put in
a (partially working) hack like this.

Richard.

> Tobias,
>
> who would like to have more time for fixing such issues.
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955