Re: [x86, patch] Add tuning options to skylake-avx512
On Fri, Apr 13, 2018 at 2:46 PM, Koval, Julia wrote: > Hi, > > This patch adds 2 tuning options to -march=skylake-avx512. Ok for trunk? > > gcc/ >PR target/84413 >* config/i386/x86-tune.def > (X86_TUNE_SSE_UNALIGNED_LOAD_OPTIMAL, >X86_TUNE_SSE_UNALIGNED_STORE_OPTIMAL): Add m_SKYLAKE_AVX512. OK. Thanks, Uros.
Re: [PATCH] Make redirection only for target_clones: V2 (PR ipa/85329).
> On Fri, Apr 13, 2018 at 04:33:40PM +0200, Martin Liška wrote:
> > > Ah, but we emit the resolver only if we see a use of it. That sounds
> > > quite
> > > broken, resolver in each TU that uses it? Better to have one at each
> > > definition...
> > >
> > > Jakub
> > >
> >
> > So after quite some time I would need some brainstorming as I'm not sure
> > how to
> > fix that properly. Let's start how 'target' attribute works and I believe
> > it should
> > behave same as target_clones attribute:
>
> I think the target_clones behavior if you put the .resolver into
> .text. section in comdat rather than
> .text..resolver and .resolver comdat is reasonable.
>
> The thing is that both the .resolver and symbols are
> defined in the section in which the resolver is defined.
> Maybe it would be nice if the resolver was made not exported out of the TU,
> and eventually, not necessarily now for GCC8, turn the specializations into
> non-exported symbols too and put them into the same section or different
> sections of the same comdat group.
>
> For ctors and dtors we need extra care about the aliases, not sure if we can
> have an alias to ifunc symbol, or if we need to emit two ifunc symbols with
> the same resolver or what exactly.
>
> And yes, it would be nice if the target attribute multiversioning worked
> similarly to this. It would change behavior for it in ABI incompatible way,
> the question is how much work it would be as well.
>
> What you can do right now with the target attribute multiversioning and
> couldn't do after the change would be e.g.
> mv.h:
> __attribute__((target ("default"))) void foo ();
> __attribute__((target ("avx"))) void foo ();
> mv1.C:
> #include "mv.h"
> __attribute__((target ("default"))) void foo () {}
> mv2.C:
> #include "mv.h"
> __attribute__((target ("avx"))) void foo () {}
> mv3.C:
> #include "mv.h"
> void bar () { foo (); }
> You couldn't this in the semantics closer to target_clones, all the
> definitions would need to be done in the same file which is where you'd also
> get the ifunc symbol.
>
> IMHO it is worth changing the semantics anyway, because the current one
> isn't very well thought out, it doesn't really work well with comdats, can
> have too many resolvers with the associated costs, etc.
>
> Honza, do you agree?
Yes, it looks reasonable. I will give it bit more tought today (I am on a trip
with only sporadic internet access)
Honza
>
> Jakub
Re: [PATCH] Make redirection only for target_clones: V2 (PR ipa/85329).
On 04/12/2018 03:46 PM, Jan Hubicka wrote:
2018-04-12 Martin Liska
PR ipa/85329
* multiple_target.c (create_dispatcher_calls): Set apostrophes
for target_clone error message.
(separate_attrs): Add new argument and check for an emptry
string.
(expand_target_clones): Handle it.
(ipa_target_clone): Make redirection just for target_clones
functions.
gcc/testsuite/ChangeLog:
2018-04-12 Martin Liska
PR ipa/85329
* g++.dg/ext/pr85329.C: New test.
* gcc.target/i386/mvc12.c: New test.
@@ -413,7 +426,11 @@ expand_target_clones (struct cgraph_node *node, bool
definition)
tree attributes = make_attribute ("target", "default",
DECL_ATTRIBUTES (node->decl));
DECL_ATTRIBUTES (node->decl) = attributes;
+ DECL_COMDAT (node->decl) = 0;
+ DECL_WEAK (node->decl) = 0;
+ DECL_ARTIFICIAL (node->decl) = 1;
node->local.local = false;
+ node->set_comdat_group (NULL);
If you make C++ inline and get the idea to use target cloning attribute on this,
this will likely lead to link error if you compile multiple files because you
turn comdat to non-comdat.
Can you please explain this in more detail? Ideally showing a test-case that
would fail?
Martin
For comdats this woudl effectivly need to become C++ abi extension and we would
need to define comdat sections for these. Perhaps easiest way is to simply
reject the attribute on comdats and probaby also extern functions?
Otherwise patch looks OK.
Honza
Re: [PATCH] Make redirection only for target_clones: V2 (PR ipa/85329).
On 04/12/2018 03:46 PM, Jan Hubicka wrote:
2018-04-12 Martin Liska
PR ipa/85329
* multiple_target.c (create_dispatcher_calls): Set apostrophes
for target_clone error message.
(separate_attrs): Add new argument and check for an emptry
string.
(expand_target_clones): Handle it.
(ipa_target_clone): Make redirection just for target_clones
functions.
gcc/testsuite/ChangeLog:
2018-04-12 Martin Liska
PR ipa/85329
* g++.dg/ext/pr85329.C: New test.
* gcc.target/i386/mvc12.c: New test.
@@ -413,7 +426,11 @@ expand_target_clones (struct cgraph_node *node, bool
definition)
tree attributes = make_attribute ("target", "default",
DECL_ATTRIBUTES (node->decl));
DECL_ATTRIBUTES (node->decl) = attributes;
+ DECL_COMDAT (node->decl) = 0;
+ DECL_WEAK (node->decl) = 0;
+ DECL_ARTIFICIAL (node->decl) = 1;
node->local.local = false;
+ node->set_comdat_group (NULL);
If you make C++ inline and get the idea to use target cloning attribute on this,
this will likely lead to link error if you compile multiple files because you
turn comdat to non-comdat.
For comdats this woudl effectivly need to become C++ abi extension and we would
need to define comdat sections for these. Perhaps easiest way is to simply
reject the attribute on comdats and probaby also extern functions?
Otherwise patch looks OK.
Honza
And Honza also asked me for description what has changes in between GCC 7.x and
current trunk.
There are test-cases that illustrate that:
./gcc/testsuite/gcc.target/i386/mvc10.c
./gcc/testsuite/gcc.target/i386/mvc11.c
./gcc/testsuite/gcc.target/i386/mvc5.c
./gcc/testsuite/gcc.target/i386/mvc7.c
./gcc/testsuite/gcc.target/i386/pr80732.c
./gcc/testsuite/gcc.target/i386/pr81214.c
Martin
Re: [PATCH] Make redirection only for target_clones: V2 (PR ipa/85329).
On Sat, Apr 14, 2018 at 11:53:51AM +0200, Martin Liška wrote:
> > > 2018-04-12 Martin Liska
> > >
> > > PR ipa/85329
> > > * g++.dg/ext/pr85329.C: New test.
> > > * gcc.target/i386/mvc12.c: New test.
> > > @@ -413,7 +426,11 @@ expand_target_clones (struct cgraph_node *node, bool
> > > definition)
> > > tree attributes = make_attribute ("target", "default",
> > > DECL_ATTRIBUTES (node->decl));
> > > DECL_ATTRIBUTES (node->decl) = attributes;
> > > + DECL_COMDAT (node->decl) = 0;
> > > + DECL_WEAK (node->decl) = 0;
> > > + DECL_ARTIFICIAL (node->decl) = 1;
> > > node->local.local = false;
> > > + node->set_comdat_group (NULL);
> >
> > If you make C++ inline and get the idea to use target cloning attribute on
> > this,
> > this will likely lead to link error if you compile multiple files because
> > you
> > turn comdat to non-comdat.
>
> Can you please explain this in more detail? Ideally showing a test-case that
> would fail?
I'd guess something along the lines of
typedef void (*F) ();
__attribute__((target ("default"))) inline void foo () {}
__attribute__((target ("avx"))) inline void foo () {}
__attribute__((target_clones ("default", "avx"))) inline void bar () {}
#ifdef SRC_A
F pa = foo;
F qa = bar;
#else
F pb = foo;
F qb = bar;
extern F pa, qa;
int
main ()
{
asm volatile ("" : "+g" (pa), "+g" (pb), "+g" (qa), "+g" (qb));
pa ();
pb ();
qa ();
qb ();
}
#endif
and
g++ -O2 -c -DSRC_A test.C -o testa.o
g++ -O2 -o test{,a.o,.C}
This doesn't actually fail, because the bar symbol, even when it doesn't
have STB_WEAK (which it should), is in the comdat group and so only one is
picked up.
Jakub
Re: [Ping, Fortran, Patch, PR81773, PR83606, coarray, v1] Fix coarray get to array with vector indexing
Hi Andre, This is OK for trunk. Thanks for the patch Paul On 13 April 2018 at 08:34, Andre Vehreschild wrote: > Ping > > On Sun, 8 Apr 2018 14:25:50 +0200 > Andre Vehreschild wrote: > >> Hi all, >> >> attached patch fixes (to my knowledge) the two PRs 81773 and 83606 where the >> result of a coarray get() operation is assigned to an array using a vector as >> index. It took me quite long to get it right, because I had to use the >> scalarizer which I haven't use directly before. So reviewers with expertise >> on >> using the scalarizer are especially welcome. >> >> Bootstrapped and regtested on x86_64-linux/f27. >> >> Ok for trunk? Backports? >> >> Regards, >> Andre > > > -- > Andre Vehreschild * Email: vehre ad gmx dot de -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
[patch, fortran] Fix PR 85387
Hello world,
the attached patch fixes the PR, an 8 regression caused by
trying to convert a nested implied DO loop to an array
for a case where this was not possible.
Regression-tested. OK for trunk?
Regards
Thomas
2018-04-14 Thomas Koenig
PR fortran/85387
* frontend-passes.c (traverse_io_block): Check for start, end or
stride being defined by an outer implied DO loop.
2018-04-14 Thomas Koenig
PR fortran/85387
* gfortran.dg/implied_do_io_5.f90: New test.
Index: frontend-passes.c
===
--- frontend-passes.c (Revision 259222)
+++ frontend-passes.c (Arbeitskopie)
@@ -1237,6 +1237,23 @@ traverse_io_block (gfc_code *code, bool *has_reach
}
}
+ /* Check for cases like ((a(i, j), i=1, j), j=1, 2). */
+ for (int i = 1; i < ref->u.ar.dimen; i++)
+{
+ if (iters[i])
+ {
+ gfc_expr *var = iters[i]->var;
+ for (int j = i - 1; j < i; j++)
+ {
+ if (iters[j]
+ && (gfc_check_dependency (var, iters[j]->start, true)
+ || gfc_check_dependency (var, iters[j]->end, true)
+ || gfc_check_dependency (var, iters[j]->step, true)))
+ return false;
+ }
+ }
+}
+
/* Create new expr. */
new_e = gfc_copy_expr (curr->expr1);
new_e->expr_type = EXPR_VARIABLE;
! { dg-do run }
! { dg-additional-options "-ffrontend-optimize" }
! PR fortran/85387 - incorrect output
! Original test case by Vittorio Zecca
program main
real :: efg_pw(2,2)
character (len=80) :: c1, c2
efg_pw(1,1)=1
efg_pw(2,1)=2
efg_pw(1,2)=3
efg_pw(2,2)=4
write (unit=c1,fmt='(3F12.5)') ((efg_pw(i, j), i=1, j), j=1, 2)
write (unit=c2,fmt='(3F12.5)') 1.0, 3.0, 4.0
if (c1 /= c2) stop 1
end
Re: [patch, fortran] Fix PR 85387
Hi Thomas, this looks good. Ok for trunk. Thanks for the patch. - Andre On Sat, 14 Apr 2018 13:35:37 +0200 Thomas König wrote: > Hello world, > > the attached patch fixes the PR, an 8 regression caused by > trying to convert a nested implied DO loop to an array > for a case where this was not possible. > > Regression-tested. OK for trunk? > > Regards > > Thomas > > 2018-04-14 Thomas Koenig > > PR fortran/85387 > * frontend-passes.c (traverse_io_block): Check for start, end or > stride being defined by an outer implied DO loop. > > 2018-04-14 Thomas Koenig > > PR fortran/85387 > * gfortran.dg/implied_do_io_5.f90: New test. -- Andre Vehreschild * Email: vehre ad gmx dot de
Re: [patch, fortran] Fix PR 85387
Hi Andre, this looks good. Ok for trunk. Thanks for the patch. Committed as r259384. Thanks for the quick review! Looking at the serious regressions from the gcc home page, we are fast approaching the gcc 8 release. Serious regressions are below 100, and there are currently only So, if anybody has any bugs that should urgently be fixed for the release, please go ahead :-) Regards Thomas
Display priority in "Serious" bugs for gcc 8 from web page
Hello world, whenever I look at the list of serious bugs, I find myself chaning the columns to add the priority field. What do you think about adding the priority field when clicking on that link? A patch is attached. Regards Thomas Index: index.html === RCS file: /cvs/gcc/wwwdocs/htdocs/index.html,v retrieving revision 1.1080 diff -u -r1.1080 index.html --- index.html 2 Apr 2018 15:22:36 - 1.1080 +++ index.html 14 Apr 2018 14:00:29 - @@ -162,7 +162,7 @@ href="https://gcc.gnu.org/bugzilla/buglist.cgi?query_format=advanced&short_desc_type=regexp&short_desc=%5C[([ 0-9.%2F]*[ %2F])*8[ %2F][ 0-9.%2F]*[Rr]egression *%5C]&target_milestone=6.5&target_milestone=7.4&target_milestone=8.0&known_to_fail_type=allwordssubstr&known_to_work_type=allwordssubstr&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&gcchost_type=allwordssubstr&gcchost=&gcctarget_type=allwordssubstr&gcctarget=&gccbuild_type=allwordssubstr&gccbuild=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&priority=P1&priority=P2&priority=P3&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=">Serious regressions. https://gcc.gnu.org/bugzilla/buglist.cgi?query_format=advanced&short_desc_type=regexp&short_desc=%5C[([ 0-9.%2F]*[ %2F])*8[ %2F][ 0-9.%2F]*[Rr]egression *%5C]&target_milestone=6.5&target_milestone=7.4&target_milestone=8.0&known_to_fail_type=allwordssubstr&known_to_work_type=allwordssubstr&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&gcchost_type=allwordssubstr&gcchost=&gcctarget_type=allwordssubstr&gcctarget=&gccbuild_type=allwordssubstr&gccbuild=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=">All + href="https://gcc.gnu.org/bugzilla/buglist.cgi?query_format=advanced&short_desc_type=regexp&short_desc=%5C[([ 0-9.%2F]*[ %2F])*8[ %2F][ 0-9.%2F]*[Rr]egression *%5C]&target_milestone=6.5&target_milestone=7.4&target_milestone=8.0&known_to_fail_type=allwordssubstr&known_to_work_type=allwordssubstr&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&gcchost_type=allwordssubstr&gcchost=&gcctarget_type=allwordssubstr&gcctarget=&gccbuild_type=allwordssubstr&gccbuild=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=&columnlist=product%2Ccomponent%2Cpriority%2Cassigned_to%2Cbug_status%2Cresolution%2Cshort_desc%2Cchangeddate">All regressions.
Re: [Ping, Fortran, Patch, PR81773, PR83606, coarray, v1] Fix coarray get to array with vector indexing
Hi Paul,
thank you for the review. Committed as r259385.
Regards,
Andre
On Sat, 14 Apr 2018 11:53:44 +0100
Paul Richard Thomas wrote:
> Hi Andre,
>
> This is OK for trunk.
>
> Thanks for the patch
>
> Paul
>
>
> On 13 April 2018 at 08:34, Andre Vehreschild wrote:
> > Ping
> >
> > On Sun, 8 Apr 2018 14:25:50 +0200
> > Andre Vehreschild wrote:
> >
> >> Hi all,
> >>
> >> attached patch fixes (to my knowledge) the two PRs 81773 and 83606 where
> >> the result of a coarray get() operation is assigned to an array using a
> >> vector as index. It took me quite long to get it right, because I had to
> >> use the scalarizer which I haven't use directly before. So reviewers with
> >> expertise on using the scalarizer are especially welcome.
> >>
> >> Bootstrapped and regtested on x86_64-linux/f27.
> >>
> >> Ok for trunk? Backports?
> >>
> >> Regards,
> >> Andre
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>
>
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
Index: gcc/fortran/ChangeLog
===
--- gcc/fortran/ChangeLog (Revision 259384)
+++ gcc/fortran/ChangeLog (Arbeitskopie)
@@ -1,3 +1,15 @@
+2018-04-14 Andre Vehreschild
+
+ PR fortran/81773
+ PR fortran/83606
+ * dependency.c (gfc_dep_resolver): Coarray indexes are to be ignored
+ during dependency computation. They define no data dependency.
+ * trans-array.c (conv_array_index_offset): The stride can not be set
+ here, prevent fail.
+ * trans-intrinsic.c (conv_caf_send): Add creation of temporary array
+ for caf_get's result and copying to the array with vectorial
+ indexing.
+
2018-04-14 Thomas Koenig
PR fortran/85387
Index: gcc/fortran/dependency.c
===
--- gcc/fortran/dependency.c (Revision 259384)
+++ gcc/fortran/dependency.c (Arbeitskopie)
@@ -2238,8 +2238,9 @@
break;
/* Exactly matching and forward overlapping ranges don't cause a
- dependency. */
- if (fin_dep < GFC_DEP_BACKWARD)
+ dependency, when they are not part of a coarray ref. */
+ if (fin_dep < GFC_DEP_BACKWARD
+ && lref->u.ar.codimen == 0 && rref->u.ar.codimen == 0)
return 0;
/* Keep checking. We only have a dependency if
Index: gcc/fortran/trans-array.c
===
--- gcc/fortran/trans-array.c (Revision 259384)
+++ gcc/fortran/trans-array.c (Arbeitskopie)
@@ -3215,7 +3215,7 @@
}
/* Multiply by the stride. */
- if (!integer_onep (stride))
+ if (stride != NULL && !integer_onep (stride))
index = fold_build2_loc (input_location, MULT_EXPR, gfc_array_index_type,
index, stride);
Index: gcc/fortran/trans-intrinsic.c
===
--- gcc/fortran/trans-intrinsic.c (Revision 259384)
+++ gcc/fortran/trans-intrinsic.c (Arbeitskopie)
@@ -1907,34 +1907,124 @@
}
else
{
- /* If has_vector, pass descriptor for whole array and the
- vector bounds separately. */
- gfc_array_ref *ar, ar2;
- bool has_vector = false;
+ bool has_vector = gfc_has_vector_subscript (lhs_expr);
- if (gfc_is_coindexed (lhs_expr) && gfc_has_vector_subscript (lhs_expr))
+ if (gfc_is_coindexed (lhs_expr) || !has_vector)
{
- has_vector = true;
- ar = gfc_find_array_ref (lhs_expr);
- ar2 = *ar;
- memset (ar, '\0', sizeof (*ar));
- ar->as = ar2.as;
- ar->type = AR_FULL;
+ /* If has_vector, pass descriptor for whole array and the
+ vector bounds separately. */
+ gfc_array_ref *ar, ar2;
+ bool has_tmp_lhs_array = false;
+ if (has_vector)
+ {
+ has_tmp_lhs_array = true;
+ ar = gfc_find_array_ref (lhs_expr);
+ ar2 = *ar;
+ memset (ar, '\0', sizeof (*ar));
+ ar->as = ar2.as;
+ ar->type = AR_FULL;
+ }
+ lhs_se.want_pointer = 1;
+ gfc_conv_expr_descriptor (&lhs_se, lhs_expr);
+ /* Using gfc_conv_expr_descriptor, we only get the descriptor, but
+ that has the wrong type if component references are done. */
+ lhs_type = gfc_typenode_for_spec (&lhs_expr->ts);
+ tmp = build_fold_indirect_ref_loc (input_location, lhs_se.expr);
+ gfc_add_modify (&lhs_se.pre, gfc_conv_descriptor_dtype (tmp),
+ gfc_get_dtype_rank_type (has_vector ? ar2.dimen
+ : lhs_expr->rank,
+ lhs_type));
+ if (has_tmp_lhs_array)
+ {
+ vec = conv_caf_vector_subscript (&block, lhs_se.expr, &ar2);
+ *ar = ar2;
+ }
}
- lhs_se.want_pointer = 1;
- gfc_conv_expr_descriptor (&lhs_se, lhs_expr);
- /* Using gfc_conv_expr_descriptor, we only get the descriptor, but that
- has the wrong type if component references are done. */
- lhs_type = gfc_typenode_for_spec (&lhs_expr->ts);
- tmp = build_fold_indirect_ref_loc (input_location, lhs_se.expr);
- gfc_add_modify (&lhs_se.pre, gfc_co
