Re: [PATCH 1/2] Fortran: Cleanup struct ext_attr_t
Le 21/11/2022 à 21:34, Bernhard Reutner-Fischer a écrit : On Mon, 21 Nov 2022 12:08:20 +0100 Mikael Morin wrote: * gfortran.h (struct ext_attr_t): Remove middle_end_name. * trans-decl.cc (add_attributes_to_decl): Move building tree_list to ... * decl.cc (gfc_match_gcc_attributes): ... here. Add the attribute to the tree_list for the middle end. I prefer to not do any middle-end stuff at parsing time, so I would rather not do this change. Not OK. Ok, that means we should filter-out those bits that we don't want to write to the module (right?). We've plenty of bits left, more than Dave Love would want to have added, i hope, so that should not be much of a concern. I didn't think of modules. Yes, that means we have to store (in memory) the attribute we have parsed, and we can filter-out the attributes at the time the attributes are written to the module. I don't think it is strictly necessary (for flatten, at least) though. What that table really wants to say is whether or not this attribute should be passed to the ME. Would it be acceptable to remove these duplicate strings and just have a bool/char/int that is true if it should be lowered (in trans-decl, as before)? But now i admit it's just bikeshedding and we can as well leave it alone for now.. It was just a though. Yes, that would be acceptable.
Re: [PATCH 1/2] symtab: also change RTL decl name
> On Mon, 21 Nov 2022 20:02:49 +0100
> Jan Hubicka wrote:
>
> > > Hi Honza, Ping.
> > > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
> > > Ok?
> > > I'd need this for attribute target_clones for the Fortran FE.
> > Sorry for delay here.
> > > > void
> > > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree
> > > > decl, tree name)
> > > > warning (0, "%qD renamed after being referenced in assembly",
> > > > decl);
> > > >
> > > >SET_DECL_ASSEMBLER_NAME (decl, name);
> > > > + /* Set the new name in rtl. */
> > > > + if (DECL_RTL_SET_P (decl))
> > > > + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER
> > > > (name);
> >
> > I am not quite sure how safe this is. We generally produce DECL_RTL
> > when we produce assembly file. So if DECL_RTL is set then we probably
> > already output the original function name and it is too late to change
> > it.
>
> AFAICS we make_decl_rtl in the fortran FE in trans_function_start.
I see, it may be a relic of something that is no longer necessary. Can
you see why one needs DECL_RTL so early?
>
> >
> > Also RTL is shared so changing it in-place is going to rewrite all the
> > existing RTL expressions using it.
> >
> > Why the DECL_RTL is produced for function you want to rename?
>
> I think the fortran FE sets it quite early when lowering a function.
> Later, when the ME creates the target_clones, it wants to rename the
> initial function to initial_fun.default for the default target.
> That's where the change_decl_assembler_name is called (only on the
> decl).
> But nobody changes the RTL name, so the ifunc (which should be the
> initial, unchanged name) is properly emitted but
> assemble_start_function uses the same, unchanged, initial fnname it
> later obtains by get_fnname_from_decl which fetches the (wrong) initial
> name where it should use the .default target name.
> See
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605081.html
>
> I'm open to other suggestions to make this work in a different way, of
> course. Maybe we're missing some magic somewhere that might share the
> name between the fndecl and the RTL XSTR so the RTL is magically
> updated by that single SET_ECL_ASSEMBLER_NAME in
> change_decl_assembler_name? But i didn't quite see where that'd be?
I think we should start by understanding why Fortran FE produces
DECL_RTL early. It was written before symbol table code emerged, it may
be simply an oversight I made while converting FE to symbol table.
Honza
>
> thanks,
>
> > Honza
> > > > +
> > > >if (alias)
> > > > {
> > > > IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;
> > >
>
Re: [PATCH 2/2] Fortran: add attribute target_clones
Le 21/11/2022 à 23:26, Bernhard Reutner-Fischer a écrit :
On Mon, 21 Nov 2022 20:13:40 +0100
Mikael Morin wrote:
Hello,
Le 09/11/2022 à 20:02, Bernhard Reutner-Fischer via Fortran a écrit :
Hi!
(...)
+ if (allow_multiple && gfc_match_char (')') != MATCH_YES)
+{
+ gfc_error ("expected ')' at %C");
+ return NULL_TREE;
+}
+
+ return attr_args;
+}
I'm not sure this function need to do all the parsing manually.
I would rather use gfc_match_actual_arglist, or maybe implement the
function as a wrapper around it.
What is allowed here? Are non-literal constants allowed, for example
parameter variables? Is line continuation supported ?
Line continuation is supported i think.
Parameter variables supposedly are or should not be supported. Why would
you do that in the context of an attribute target decl? > Either way, if the ME
does not find such an fndecl, it will complain
and ignore the attribute.
I don't understand non-literal constants in this context.
This very attribute applies to decls, so the existing code supposedly
matches a comma separated list of identifiers. The usual dollar-ok
caveats apply.
No, my comment and my questions were about your function, which, as I
understand it, matches the arguments to the attribute: it matches open
and closing parenthesis, double quotes, etc.
Matching of decl names comes after that.
I asked the question about non-literal constant (and the other as well),
because I saw it as a possible reason to not reuse the existing parsing
functions.
As to gfc_match_actual_arglist, probably.
target_clones has
+ { "target_clones", 1, -1, true, false, false, false,
+ dummy, NULL },
with tree-core.h struct attribute_spec, so
name, min=1, max=unbounded, decl_required=yes, ...ignore...
hence applies to functions and subroutines and the like. It does take an
unbounded list of strings, isa1, isa2, isa4, default. We could add
"default" unless seen, but i'd rather want it spelled out by the user
for the user is supposed to know what she's doing, as in c or c++.
The ME has code to sanity-check the attributes, including conflicting
(ME) attributes.
The reason why i contemplated with a separate parser was that for stuff
like regparm or sseregparm, you would want to require a single number
for the equivalent of
__attribute__((regparm(3),stdcall)
which you would provide in 2 separate !GCC$ attributes i assume.
Well, the check could as easily be enforced after parsing with the
existing parsing functions.
Nothing (bad) to say about the rest, but there is enough to change with
the above comments.
Yes, many thanks for your comments.
I think there is no other non-intrusive way to pass the data through the
frontend. So for an acceptable way this means touching quite some spots
for every single ME attribute anybody would like to add in the future.
I'm not sure I understand. Please let's just add what is necessary for
this attribute, not more.
Re: typespec in forall and implied-do
Minor addition: program foo implicit none real(8) :: i integer, parameter :: q(*) = [(kind(i), integer :: i = 1, 3)] print *, q end program foo This prints 8 8 8 although it should be all 4's. So we really need to create a local namespace or even block to shadow the host's variable. Crayftn and NAG accept this too, Intel has a problem report on this. Harald
[PATCH] Fortran: error recovery on associate with bad selector [PR107577]
Dear all,
please find attached an obvious patch by Steve for a technical
regression that resulted from improvements in error recovery
of bad uses of associate.
Regtested on x86_64-pc-linux-gnu.
Will commit soon unless there are comments.
As a sidenote: the testcase shows that we resolve the associate
names quite often, likely more often than necessary, resulting
in many error messages produced for the same line of code. In
the present case, each use of the bad name produces two errors,
one where it is used, and one at the associate statement.
That is probably not helpful for the user.
Thanks,
Harald
From 9ff8d2ec56d139b54e2f66f747142687a38d2106 Mon Sep 17 00:00:00 2001
From: Steve Kargl
Date: Tue, 22 Nov 2022 22:31:51 +0100
Subject: [PATCH] Fortran: error recovery on associate with bad selector
[PR107577]
gcc/fortran/ChangeLog:
PR fortran/107577
* resolve.cc (find_array_spec): Choose appropriate locus either of
bad array reference or of non-array entity in error message.
gcc/testsuite/ChangeLog:
PR fortran/107577
* gfortran.dg/pr107577.f90: New test.
---
gcc/fortran/resolve.cc | 3 ++-
gcc/testsuite/gfortran.dg/pr107577.f90 | 13 +
2 files changed, 15 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr107577.f90
diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index 24e5aa03556..3396c6ce4a7 100644
--- a/gcc/fortran/resolve.cc
+++ b/gcc/fortran/resolve.cc
@@ -5005,8 +5005,9 @@ find_array_spec (gfc_expr *e)
case REF_ARRAY:
if (as == NULL)
{
+ locus loc = ref->u.ar.where.lb ? ref->u.ar.where : e->where;
gfc_error ("Invalid array reference of a non-array entity at %L",
- &ref->u.ar.where);
+ &loc);
return false;
}
diff --git a/gcc/testsuite/gfortran.dg/pr107577.f90 b/gcc/testsuite/gfortran.dg/pr107577.f90
new file mode 100644
index 000..94e6620a0ee
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr107577.f90
@@ -0,0 +1,13 @@
+! { dg-do compile }
+! PR fortran/107577 - ICE in find_array_spec
+! Contributed by G.Steinmetz
+
+program p
+ implicit none
+ associate (y => f(4))! { dg-error "has no IMPLICIT type" }
+if (lbound (y, 1) /= 1) stop 1 ! { dg-error "Invalid array reference" }
+if (y(1) /= 1) stop 2 ! { dg-error "Invalid array reference" }
+ end associate
+end
+
+! { dg-error "has no type" " " { target *-*-* } 7 }
--
2.35.3
Re: typespec in forall and implied-do
On Tue, Nov 22, 2022 at 10:15:39PM +0100, Harald Anlauf via Fortran wrote: > Minor addition: > > program foo > implicit none > real(8) :: i > integer, parameter :: q(*) = [(kind(i), integer :: i = 1, 3)] > print *, q > end program foo > > This prints > >8 8 8 > > although it should be all 4's. So we really need to create a local > namespace or even block to shadow the host's variable. > > Crayftn and NAG accept this too, Intel has a problem report on this. > I'll see if I can make the shadow variable idea work. For two lines integer, parameter :: q(3) = [(kind(i), integer :: i = 1, 3)] integer:: p(3) = [(kind(i), integer :: i = 1, 3)] I believe the paths through the compiler differ sufficiently, and the shado variable might help in keeping the change simple. -- Steve
