Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Jeff Law [2016-11-29 10:35:50 -0700]:
> On 11/29/2016 07:02 AM, Andrew Burgess wrote:
> > * Jeff Law [2016-11-28 15:08:46 -0700]:
> >
> > > On 11/24/2016 02:40 PM, Andrew Burgess wrote:
> > > > * Christophe Lyon [2016-11-21 13:47:09
> > > > +0100]:
> > > >
> > > > > On 20 November 2016 at 18:27, Mike Stump
> > > > > wrote:
> > > > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess
> > > > > > wrote:
> > > > > > > > So, your new test fails on arm* targets:
> > > > > > >
> > > > > > > After a little digging I think the problem might be that
> > > > > > > -freorder-blocks-and-partition is not supported on arm.
> > > > > > >
> > > > > > > This should be detected as the new tests include:
> > > > > > >
> > > > > > >/* { dg-require-effective-target freorder } */
> > > > > > >
> > > > > > > however this test passed on arm as -freorder-blocks-and-partition
> > > > > > > does
> > > > > > > not issue any warning unless -fprofile-use is also passed.
> > > > > > >
> > > > > > > The patch below extends check_effective_target_freorder to check
> > > > > > > using
> > > > > > > -fprofile-use. With this change in place the tests are skipped on
> > > > > > > arm.
> > > > > >
> > > > > > > All feedback welcome,
> > > > > >
> > > > > > Seems reasonable, unless a
> > > > > > -freorder-blocks-and-partition/-fprofile-use person thinks this is
> > > > > > the wrong solution.
> > > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > As promised, I tested this patch: it makes
> > > > > gcc.dg/tree-prof/section-attr-[123].c
> > > > > unsupported on arm*, and thus they are not failing anymore :-)
> > > > >
> > > > > However, it also makes other tests unsupported, while they used to
> > > > > pass:
> > > > >
> > > > > gcc.dg/pr33648.c
> > > > > gcc.dg/pr46685.c
> > > > > gcc.dg/tree-prof/20041218-1.c
> > > > > gcc.dg/tree-prof/bb-reorg.c
> > > > > gcc.dg/tree-prof/cold_partition_label.c
> > > > > gcc.dg/tree-prof/comp-goto-1.c
> > > > > gcc.dg/tree-prof/pr34999.c
> > > > > gcc.dg/tree-prof/pr45354.c
> > > > > gcc.dg/tree-prof/pr50907.c
> > > > > gcc.dg/tree-prof/pr52027.c
> > > > > gcc.dg/tree-prof/va-arg-pack-1.c
> > > > >
> > > > > and failures are now unsupported:
> > > > > gcc.dg/tree-prof/cold_partition_label.c
> > > > > gcc.dg/tree-prof/section-attr-1.c
> > > > > gcc.dg/tree-prof/section-attr-2.c
> > > > > gcc.dg/tree-prof/section-attr-3.c
> > > > >
> > > > > So, maybe this patch is too strong?
> > > >
> > > > In all of the cases that used to pass the tests are compile only tests
> > > > (except for cold_partition_label, which I discuss below).
> > > >
> > > > On ARM passing -fprofile-use and -freorder-blocks-and-partition
> > > > results in a warning, and the -freorder-blocks-and-partition flag is
> > > > ignored. However, disabling -freorder-blocks-and-partition doesn't
> > > > stop any of the tests compiling, hence the passes.
> > > >
> > > > All the tests include:
> > > >
> > > > /* { dg-require-effective-target freorder } */
> > > >
> > > > which I understand to mean, the tests requires the 'freorder' feature
> > > > to be supported (which corresponds to -freorder-blocks-and-partition).
> > > >
> > > > For cold_partition_label and my new tests it's seems clear that the
> > > > lack of support for -freorder-blocks-and-partition on ARM is the cause
> > > > of the test failures.
> > > >
> > > > So, is it reasonable to give up the other tests as "unsupported"? I'd
> > > > be inclined to say yes, but I happy to rework the patch if anyone has
> > > > a suggestion for an alternative approach.
> > > It is reasonable. It's not uncommon to have to drop various tests to
> > > UNSUPPORTED, particularly things which depend on assembler/linker
> > > capabilities, the target runtime system, etc.
> >
> > OK, I'm going to take that as approval for my patch[1]. I'll wait a
> > couple of days to give people a chance to correct me, then I'll push
> > the change. This should resolve the test regressions I introduced for
> > ARM.
> I'll just go ahead and explicitly ACK this.
Committed as r243009.
Thanks,
Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 11/29/2016 07:02 AM, Andrew Burgess wrote:
* Jeff Law [2016-11-28 15:08:46 -0700]:
On 11/24/2016 02:40 PM, Andrew Burgess wrote:
* Christophe Lyon [2016-11-21 13:47:09 +0100]:
On 20 November 2016 at 18:27, Mike Stump wrote:
On Nov 19, 2016, at 1:59 PM, Andrew Burgess wrote:
So, your new test fails on arm* targets:
After a little digging I think the problem might be that
-freorder-blocks-and-partition is not supported on arm.
This should be detected as the new tests include:
/* { dg-require-effective-target freorder } */
however this test passed on arm as -freorder-blocks-and-partition does
not issue any warning unless -fprofile-use is also passed.
The patch below extends check_effective_target_freorder to check using
-fprofile-use. With this change in place the tests are skipped on
arm.
All feedback welcome,
Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person
thinks this is the wrong solution.
Hi,
As promised, I tested this patch: it makes
gcc.dg/tree-prof/section-attr-[123].c
unsupported on arm*, and thus they are not failing anymore :-)
However, it also makes other tests unsupported, while they used to pass:
gcc.dg/pr33648.c
gcc.dg/pr46685.c
gcc.dg/tree-prof/20041218-1.c
gcc.dg/tree-prof/bb-reorg.c
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/comp-goto-1.c
gcc.dg/tree-prof/pr34999.c
gcc.dg/tree-prof/pr45354.c
gcc.dg/tree-prof/pr50907.c
gcc.dg/tree-prof/pr52027.c
gcc.dg/tree-prof/va-arg-pack-1.c
and failures are now unsupported:
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/section-attr-1.c
gcc.dg/tree-prof/section-attr-2.c
gcc.dg/tree-prof/section-attr-3.c
So, maybe this patch is too strong?
In all of the cases that used to pass the tests are compile only tests
(except for cold_partition_label, which I discuss below).
On ARM passing -fprofile-use and -freorder-blocks-and-partition
results in a warning, and the -freorder-blocks-and-partition flag is
ignored. However, disabling -freorder-blocks-and-partition doesn't
stop any of the tests compiling, hence the passes.
All the tests include:
/* { dg-require-effective-target freorder } */
which I understand to mean, the tests requires the 'freorder' feature
to be supported (which corresponds to -freorder-blocks-and-partition).
For cold_partition_label and my new tests it's seems clear that the
lack of support for -freorder-blocks-and-partition on ARM is the cause
of the test failures.
So, is it reasonable to give up the other tests as "unsupported"? I'd
be inclined to say yes, but I happy to rework the patch if anyone has
a suggestion for an alternative approach.
It is reasonable. It's not uncommon to have to drop various tests to
UNSUPPORTED, particularly things which depend on assembler/linker
capabilities, the target runtime system, etc.
OK, I'm going to take that as approval for my patch[1]. I'll wait a
couple of days to give people a chance to correct me, then I'll push
the change. This should resolve the test regressions I introduced for
ARM.
I'll just go ahead and explicitly ACK this.
Thanks,
jeff
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Jeff Law [2016-11-28 15:08:46 -0700]:
> On 11/24/2016 02:40 PM, Andrew Burgess wrote:
> > * Christophe Lyon [2016-11-21 13:47:09 +0100]:
> >
> > > On 20 November 2016 at 18:27, Mike Stump wrote:
> > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess
> > > > wrote:
> > > > > > So, your new test fails on arm* targets:
> > > > >
> > > > > After a little digging I think the problem might be that
> > > > > -freorder-blocks-and-partition is not supported on arm.
> > > > >
> > > > > This should be detected as the new tests include:
> > > > >
> > > > >/* { dg-require-effective-target freorder } */
> > > > >
> > > > > however this test passed on arm as -freorder-blocks-and-partition does
> > > > > not issue any warning unless -fprofile-use is also passed.
> > > > >
> > > > > The patch below extends check_effective_target_freorder to check using
> > > > > -fprofile-use. With this change in place the tests are skipped on
> > > > > arm.
> > > >
> > > > > All feedback welcome,
> > > >
> > > > Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use
> > > > person thinks this is the wrong solution.
> > > >
> > >
> > > Hi,
> > >
> > > As promised, I tested this patch: it makes
> > > gcc.dg/tree-prof/section-attr-[123].c
> > > unsupported on arm*, and thus they are not failing anymore :-)
> > >
> > > However, it also makes other tests unsupported, while they used to pass:
> > >
> > > gcc.dg/pr33648.c
> > > gcc.dg/pr46685.c
> > > gcc.dg/tree-prof/20041218-1.c
> > > gcc.dg/tree-prof/bb-reorg.c
> > > gcc.dg/tree-prof/cold_partition_label.c
> > > gcc.dg/tree-prof/comp-goto-1.c
> > > gcc.dg/tree-prof/pr34999.c
> > > gcc.dg/tree-prof/pr45354.c
> > > gcc.dg/tree-prof/pr50907.c
> > > gcc.dg/tree-prof/pr52027.c
> > > gcc.dg/tree-prof/va-arg-pack-1.c
> > >
> > > and failures are now unsupported:
> > > gcc.dg/tree-prof/cold_partition_label.c
> > > gcc.dg/tree-prof/section-attr-1.c
> > > gcc.dg/tree-prof/section-attr-2.c
> > > gcc.dg/tree-prof/section-attr-3.c
> > >
> > > So, maybe this patch is too strong?
> >
> > In all of the cases that used to pass the tests are compile only tests
> > (except for cold_partition_label, which I discuss below).
> >
> > On ARM passing -fprofile-use and -freorder-blocks-and-partition
> > results in a warning, and the -freorder-blocks-and-partition flag is
> > ignored. However, disabling -freorder-blocks-and-partition doesn't
> > stop any of the tests compiling, hence the passes.
> >
> > All the tests include:
> >
> > /* { dg-require-effective-target freorder } */
> >
> > which I understand to mean, the tests requires the 'freorder' feature
> > to be supported (which corresponds to -freorder-blocks-and-partition).
> >
> > For cold_partition_label and my new tests it's seems clear that the
> > lack of support for -freorder-blocks-and-partition on ARM is the cause
> > of the test failures.
> >
> > So, is it reasonable to give up the other tests as "unsupported"? I'd
> > be inclined to say yes, but I happy to rework the patch if anyone has
> > a suggestion for an alternative approach.
> It is reasonable. It's not uncommon to have to drop various tests to
> UNSUPPORTED, particularly things which depend on assembler/linker
> capabilities, the target runtime system, etc.
OK, I'm going to take that as approval for my patch[1]. I'll wait a
couple of days to give people a chance to correct me, then I'll push
the change. This should resolve the test regressions I introduced for
ARM.
Thanks,
Andrew
[1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02050.html
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 11/24/2016 02:40 PM, Andrew Burgess wrote:
* Christophe Lyon [2016-11-21 13:47:09 +0100]:
On 20 November 2016 at 18:27, Mike Stump wrote:
On Nov 19, 2016, at 1:59 PM, Andrew Burgess wrote:
So, your new test fails on arm* targets:
After a little digging I think the problem might be that
-freorder-blocks-and-partition is not supported on arm.
This should be detected as the new tests include:
/* { dg-require-effective-target freorder } */
however this test passed on arm as -freorder-blocks-and-partition does
not issue any warning unless -fprofile-use is also passed.
The patch below extends check_effective_target_freorder to check using
-fprofile-use. With this change in place the tests are skipped on
arm.
All feedback welcome,
Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person
thinks this is the wrong solution.
Hi,
As promised, I tested this patch: it makes
gcc.dg/tree-prof/section-attr-[123].c
unsupported on arm*, and thus they are not failing anymore :-)
However, it also makes other tests unsupported, while they used to pass:
gcc.dg/pr33648.c
gcc.dg/pr46685.c
gcc.dg/tree-prof/20041218-1.c
gcc.dg/tree-prof/bb-reorg.c
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/comp-goto-1.c
gcc.dg/tree-prof/pr34999.c
gcc.dg/tree-prof/pr45354.c
gcc.dg/tree-prof/pr50907.c
gcc.dg/tree-prof/pr52027.c
gcc.dg/tree-prof/va-arg-pack-1.c
and failures are now unsupported:
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/section-attr-1.c
gcc.dg/tree-prof/section-attr-2.c
gcc.dg/tree-prof/section-attr-3.c
So, maybe this patch is too strong?
In all of the cases that used to pass the tests are compile only tests
(except for cold_partition_label, which I discuss below).
On ARM passing -fprofile-use and -freorder-blocks-and-partition
results in a warning, and the -freorder-blocks-and-partition flag is
ignored. However, disabling -freorder-blocks-and-partition doesn't
stop any of the tests compiling, hence the passes.
All the tests include:
/* { dg-require-effective-target freorder } */
which I understand to mean, the tests requires the 'freorder' feature
to be supported (which corresponds to -freorder-blocks-and-partition).
For cold_partition_label and my new tests it's seems clear that the
lack of support for -freorder-blocks-and-partition on ARM is the cause
of the test failures.
So, is it reasonable to give up the other tests as "unsupported"? I'd
be inclined to say yes, but I happy to rework the patch if anyone has
a suggestion for an alternative approach.
It is reasonable. It's not uncommon to have to drop various tests to
UNSUPPORTED, particularly things which depend on assembler/linker
capabilities, the target runtime system, etc.
jeff
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Christophe Lyon [2016-11-21 13:47:09 +0100]:
> On 20 November 2016 at 18:27, Mike Stump wrote:
> > On Nov 19, 2016, at 1:59 PM, Andrew Burgess
> > wrote:
> >>> So, your new test fails on arm* targets:
> >>
> >> After a little digging I think the problem might be that
> >> -freorder-blocks-and-partition is not supported on arm.
> >>
> >> This should be detected as the new tests include:
> >>
> >>/* { dg-require-effective-target freorder } */
> >>
> >> however this test passed on arm as -freorder-blocks-and-partition does
> >> not issue any warning unless -fprofile-use is also passed.
> >>
> >> The patch below extends check_effective_target_freorder to check using
> >> -fprofile-use. With this change in place the tests are skipped on
> >> arm.
> >
> >> All feedback welcome,
> >
> > Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use
> > person thinks this is the wrong solution.
> >
>
> Hi,
>
> As promised, I tested this patch: it makes
> gcc.dg/tree-prof/section-attr-[123].c
> unsupported on arm*, and thus they are not failing anymore :-)
>
> However, it also makes other tests unsupported, while they used to pass:
>
> gcc.dg/pr33648.c
> gcc.dg/pr46685.c
> gcc.dg/tree-prof/20041218-1.c
> gcc.dg/tree-prof/bb-reorg.c
> gcc.dg/tree-prof/cold_partition_label.c
> gcc.dg/tree-prof/comp-goto-1.c
> gcc.dg/tree-prof/pr34999.c
> gcc.dg/tree-prof/pr45354.c
> gcc.dg/tree-prof/pr50907.c
> gcc.dg/tree-prof/pr52027.c
> gcc.dg/tree-prof/va-arg-pack-1.c
>
> and failures are now unsupported:
> gcc.dg/tree-prof/cold_partition_label.c
> gcc.dg/tree-prof/section-attr-1.c
> gcc.dg/tree-prof/section-attr-2.c
> gcc.dg/tree-prof/section-attr-3.c
>
> So, maybe this patch is too strong?
In all of the cases that used to pass the tests are compile only tests
(except for cold_partition_label, which I discuss below).
On ARM passing -fprofile-use and -freorder-blocks-and-partition
results in a warning, and the -freorder-blocks-and-partition flag is
ignored. However, disabling -freorder-blocks-and-partition doesn't
stop any of the tests compiling, hence the passes.
All the tests include:
/* { dg-require-effective-target freorder } */
which I understand to mean, the tests requires the 'freorder' feature
to be supported (which corresponds to -freorder-blocks-and-partition).
For cold_partition_label and my new tests it's seems clear that the
lack of support for -freorder-blocks-and-partition on ARM is the cause
of the test failures.
So, is it reasonable to give up the other tests as "unsupported"? I'd
be inclined to say yes, but I happy to rework the patch if anyone has
a suggestion for an alternative approach.
One possibility would be to split the 'freorder' feature into two, one
being 'Can -freorder-blocks-and-partition be used without causing an
error (so ARM would say yes to this)', and a new feature would be
'Does -freorder-blocks-and-partition actually _work_ on the target?'
we'd then use this in cold_partition_label and in my new tests.
Though this doesn't feel a great solution
Thanks,
Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 20 November 2016 at 18:27, Mike Stump wrote:
> On Nov 19, 2016, at 1:59 PM, Andrew Burgess
> wrote:
>>> So, your new test fails on arm* targets:
>>
>> After a little digging I think the problem might be that
>> -freorder-blocks-and-partition is not supported on arm.
>>
>> This should be detected as the new tests include:
>>
>>/* { dg-require-effective-target freorder } */
>>
>> however this test passed on arm as -freorder-blocks-and-partition does
>> not issue any warning unless -fprofile-use is also passed.
>>
>> The patch below extends check_effective_target_freorder to check using
>> -fprofile-use. With this change in place the tests are skipped on
>> arm.
>
>> All feedback welcome,
>
> Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use
> person thinks this is the wrong solution.
>
Hi,
As promised, I tested this patch: it makes
gcc.dg/tree-prof/section-attr-[123].c
unsupported on arm*, and thus they are not failing anymore :-)
However, it also makes other tests unsupported, while they used to pass:
gcc.dg/pr33648.c
gcc.dg/pr46685.c
gcc.dg/tree-prof/20041218-1.c
gcc.dg/tree-prof/bb-reorg.c
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/comp-goto-1.c
gcc.dg/tree-prof/pr34999.c
gcc.dg/tree-prof/pr45354.c
gcc.dg/tree-prof/pr50907.c
gcc.dg/tree-prof/pr52027.c
gcc.dg/tree-prof/va-arg-pack-1.c
and failures are now unsupported:
gcc.dg/tree-prof/cold_partition_label.c
gcc.dg/tree-prof/section-attr-1.c
gcc.dg/tree-prof/section-attr-2.c
gcc.dg/tree-prof/section-attr-3.c
So, maybe this patch is too strong?
Christophe
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On Nov 19, 2016, at 1:59 PM, Andrew Burgess wrote:
>> So, your new test fails on arm* targets:
>
> After a little digging I think the problem might be that
> -freorder-blocks-and-partition is not supported on arm.
>
> This should be detected as the new tests include:
>
>/* { dg-require-effective-target freorder } */
>
> however this test passed on arm as -freorder-blocks-and-partition does
> not issue any warning unless -fprofile-use is also passed.
>
> The patch below extends check_effective_target_freorder to check using
> -fprofile-use. With this change in place the tests are skipped on
> arm.
> All feedback welcome,
Seems reasonable, unless a -freorder-blocks-and-partition/-fprofile-use person
thinks this is the wrong solution.
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Christophe Lyon [2016-11-18 13:21:50 +0100]:
> On 16 November 2016 at 23:12, Andrew Burgess
> wrote:
> > * Mike Stump [2016-11-16 12:59:53 -0800]:
> >
> >> On Nov 16, 2016, at 12:09 PM, Andrew Burgess
> >> wrote:
> >> > My only remaining concern is the new tests, I've tried to restrict
> >> > them to targets that I suspect they'll pass on with:
> >> >
> >> >/* { dg-final-use { scan-assembler "\.section\[\t
> >> > \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" {
> >> > target *-*-linux* *-*-gnu* } } } */
> >> >
> >> > but I'm still nervous that I'm going to introduce test failures. Is
> >> > there any advice / guidance I should follow before I commit, or are
> >> > folk pretty relaxed so long as I've made a reasonable effort?
> >>
> >> So, if you are worried about the way the line is constructed, I usually
> >> test it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux*
> >> *-*-gnNOTu* and see if the test then doesn't run on your machine. If it
> >> doesn't then you can be pretty confident that only machines that match the
> >> target triplet can be impacted. I usually do this type of testing by
> >> running the test case in isolation (not the full tests suite). Anyway, do
> >> the best you can, and don't worry about t it too much, learn from the
> >> experience, even if it goes wrong in some way. If it did go wrong, just
> >> be responsive (don't check it in just before a 6 week vacation) about
> >> fixing it, if you can.
> >>
> >
> > Thanks for the feedback.
> >
> > Change committed as revision 242519. If anyone sees any issues just
> > let me know.
> >
>
> Hi Andrew,
>
> Sorry for the delay, there are so many commits these days, with so
> many regression
> reports to check manually before reporting here
>
> So, your new test fails on arm* targets:
After a little digging I think the problem might be that
-freorder-blocks-and-partition is not supported on arm.
This should be detected as the new tests include:
/* { dg-require-effective-target freorder } */
however this test passed on arm as -freorder-blocks-and-partition does
not issue any warning unless -fprofile-use is also passed.
The patch below extends check_effective_target_freorder to check using
-fprofile-use. With this change in place the tests are skipped on
arm.
All feedback welcome,
Thanks,
Andrew
---
arm/gcc: Tighten checks in check_effective_target_freorder
In check_effective_target_freorder we check to see if the target
supports -freorder-blocks-and-partition. However we disable
-freorder-blocks-and-partition when -fprofile-use is not supplied so
for some targets we'll not see any message about lack of support for
-freorder-blocks-and-partition unless -fprofile-use is also passed.
This commit extends check_effective_target_freorder to first try
-freorder-blocks-and-partition on its own, then try -fprofile-use and
-freorder-blocks-and-partition.
gcc/testsuite/ChangeLog:
* lib/target-supports.exp (check_effective_target_freorder): Check
additional case.
---
gcc/testsuite/ChangeLog | 5 +
gcc/testsuite/lib/target-supports.exp | 8 +++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/lib/target-supports.exp
b/gcc/testsuite/lib/target-supports.exp
index 8a2abd2..0716792 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -1014,9 +1014,15 @@ proc check_effective_target_fstack_protector {} {
# for trivial code, 0 otherwise.
proc check_effective_target_freorder {} {
-return [check_no_compiler_messages freorder object {
+if { [check_no_compiler_messages freorder object {
void foo (void) { }
} "-freorder-blocks-and-partition"]
+&& [check_no_compiler_messages fprofile_use_freorder object {
+ void foo (void) { }
+} "-fprofile-use -freorder-blocks-and-partition"] } {
+ return 1
+}
+return 0
}
# Return 1 if -fpic and -fPIC are supported, as in no warnings or errors
--
2.6.4
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 16 November 2016 at 23:12, Andrew Burgess
wrote:
> * Mike Stump [2016-11-16 12:59:53 -0800]:
>
>> On Nov 16, 2016, at 12:09 PM, Andrew Burgess
>> wrote:
>> > My only remaining concern is the new tests, I've tried to restrict
>> > them to targets that I suspect they'll pass on with:
>> >
>> >/* { dg-final-use { scan-assembler "\.section\[\t
>> > \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target
>> > *-*-linux* *-*-gnu* } } } */
>> >
>> > but I'm still nervous that I'm going to introduce test failures. Is
>> > there any advice / guidance I should follow before I commit, or are
>> > folk pretty relaxed so long as I've made a reasonable effort?
>>
>> So, if you are worried about the way the line is constructed, I usually test
>> it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu*
>> and see if the test then doesn't run on your machine. If it doesn't then
>> you can be pretty confident that only machines that match the target triplet
>> can be impacted. I usually do this type of testing by running the test case
>> in isolation (not the full tests suite). Anyway, do the best you can, and
>> don't worry about t it too much, learn from the experience, even if it goes
>> wrong in some way. If it did go wrong, just be responsive (don't check it
>> in just before a 6 week vacation) about fixing it, if you can.
>>
>
> Thanks for the feedback.
>
> Change committed as revision 242519. If anyone sees any issues just
> let me know.
>
Hi Andrew,
Sorry for the delay, there are so many commits these days, with so
many regression
reports to check manually before reporting here
So, your new test fails on arm* targets:
gcc.dg/tree-prof/section-attr-1.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0
gcc.dg/tree-prof/section-attr-2.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0
gcc.dg/tree-prof/section-attr-3.c scan-assembler .section[\t
]*.text.unlikely[\\n\\r]+[\t ]*.size[\t ]*foo.cold.0
Christophe
> Thanks,
> Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 11/16/2016 03:12 PM, Andrew Burgess wrote:
* Mike Stump [2016-11-16 12:59:53 -0800]:
On Nov 16, 2016, at 12:09 PM, Andrew Burgess
wrote:
My only remaining concern is the new tests, I've tried to restrict
them to targets that I suspect they'll pass on with:
/* { dg-final-use { scan-assembler "\.section\[\t
\]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target
*-*-linux* *-*-gnu* } } } */
but I'm still nervous that I'm going to introduce test failures. Is
there any advice / guidance I should follow before I commit, or are
folk pretty relaxed so long as I've made a reasonable effort?
So, if you are worried about the way the line is constructed, I usually test it
by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and
see if the test then doesn't run on your machine. If it doesn't then you can
be pretty confident that only machines that match the target triplet can be
impacted. I usually do this type of testing by running the test case in
isolation (not the full tests suite). Anyway, do the best you can, and don't
worry about t it too much, learn from the experience, even if it goes wrong in
some way. If it did go wrong, just be responsive (don't check it in just
before a 6 week vacation) about fixing it, if you can.
Thanks for the feedback.
Change committed as revision 242519. If anyone sees any issues just
let me know.
Thanks. And thanks for seeing this through... I know it's a pain sometimes.
jeff
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Mike Stump [2016-11-16 12:59:53 -0800]:
> On Nov 16, 2016, at 12:09 PM, Andrew Burgess
> wrote:
> > My only remaining concern is the new tests, I've tried to restrict
> > them to targets that I suspect they'll pass on with:
> >
> >/* { dg-final-use { scan-assembler "\.section\[\t
> > \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target
> > *-*-linux* *-*-gnu* } } } */
> >
> > but I'm still nervous that I'm going to introduce test failures. Is
> > there any advice / guidance I should follow before I commit, or are
> > folk pretty relaxed so long as I've made a reasonable effort?
>
> So, if you are worried about the way the line is constructed, I usually test
> it by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu*
> and see if the test then doesn't run on your machine. If it doesn't then you
> can be pretty confident that only machines that match the target triplet can
> be impacted. I usually do this type of testing by running the test case in
> isolation (not the full tests suite). Anyway, do the best you can, and don't
> worry about t it too much, learn from the experience, even if it goes wrong
> in some way. If it did go wrong, just be responsive (don't check it in just
> before a 6 week vacation) about fixing it, if you can.
>
Thanks for the feedback.
Change committed as revision 242519. If anyone sees any issues just
let me know.
Thanks,
Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On Nov 16, 2016, at 12:09 PM, Andrew Burgess
wrote:
> My only remaining concern is the new tests, I've tried to restrict
> them to targets that I suspect they'll pass on with:
>
>/* { dg-final-use { scan-assembler "\.section\[\t
> \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target
> *-*-linux* *-*-gnu* } } } */
>
> but I'm still nervous that I'm going to introduce test failures. Is
> there any advice / guidance I should follow before I commit, or are
> folk pretty relaxed so long as I've made a reasonable effort?
So, if you are worried about the way the line is constructed, I usually test it
by misspelling the *-*-linux* *-*-gnu* part as *-*-linNOTux* *-*-gnNOTu* and
see if the test then doesn't run on your machine. If it doesn't then you can
be pretty confident that only machines that match the target triplet can be
impacted. I usually do this type of testing by running the test case in
isolation (not the full tests suite). Anyway, do the best you can, and don't
worry about t it too much, learn from the experience, even if it goes wrong in
some way. If it did go wrong, just be responsive (don't check it in just
before a 6 week vacation) about fixing it, if you can.
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Bernd Schmidt [2016-11-03 13:01:32 +0100]:
> On 09/14/2016 03:00 PM, Andrew Burgess wrote:
> > In an attempt to get this patch merged (as I still think that its
> > correct) I've investigated, and documented a little more about how I
> > think things currently work. I'm sure most people reading this will
> > already know this, but hopefully, if my understanding is wrong someone
> > can point it out.
> > gcc/ChangeLog:
> >
> > * gcc/bb-reorder.c: Remove 'toplev.h' include.
> > (pass_partition_blocks::gate): No longer check
> > user_defined_section_attribute, instead check the function decl
> > for a section attribute.
> > * gcc/c-family/c-common.c (handle_section_attribute): No longer
> > set user_defined_section_attribute.
> > * gcc/final.c (rest_of_handle_final): Likewise.
> > * gcc/toplev.c: Remove definition of user_defined_section_attribute.
> > * gcc/toplev.h: Remove declaration of
> > user_defined_section_attribute.
> >
> > gcc/testsuiteChangeLog:
> >
> > * gcc.dg/tree-prof/section-attr-1.c: New file.
> > * gcc.dg/tree-prof/section-attr-2.c: New file.
> > * gcc.dg/tree-prof/section-attr-3.c: New file.
>
> I think the explanation is perfectly reasonable and the patch looks good,
> except:
>
> > +__attribute__((noinline))
>
> Add noclone to all of these as well.
Thanks. Considering Jeff said, I'm thinking about it, and you've said
yes, and given Jeff's not got back, I'm considering this patch
approved (with the fix you suggest).
My only remaining concern is the new tests, I've tried to restrict
them to targets that I suspect they'll pass on with:
/* { dg-final-use { scan-assembler "\.section\[\t
\]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target
*-*-linux* *-*-gnu* } } } */
but I'm still nervous that I'm going to introduce test failures. Is
there any advice / guidance I should follow before I commit, or are
folk pretty relaxed so long as I've made a reasonable effort?
Thanks,
Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 09/14/2016 03:00 PM, Andrew Burgess wrote: In an attempt to get this patch merged (as I still think that its correct) I've investigated, and documented a little more about how I think things currently work. I'm sure most people reading this will already know this, but hopefully, if my understanding is wrong someone can point it out. gcc/ChangeLog: * gcc/bb-reorder.c: Remove 'toplev.h' include. (pass_partition_blocks::gate): No longer check user_defined_section_attribute, instead check the function decl for a section attribute. * gcc/c-family/c-common.c (handle_section_attribute): No longer set user_defined_section_attribute. * gcc/final.c (rest_of_handle_final): Likewise. * gcc/toplev.c: Remove definition of user_defined_section_attribute. * gcc/toplev.h: Remove declaration of user_defined_section_attribute. gcc/testsuiteChangeLog: * gcc.dg/tree-prof/section-attr-1.c: New file. * gcc.dg/tree-prof/section-attr-2.c: New file. * gcc.dg/tree-prof/section-attr-3.c: New file. I think the explanation is perfectly reasonable and the patch looks good, except: +__attribute__((noinline)) Add noclone to all of these as well. Bernd
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Jeff Law [2016-10-28 09:58:14 -0600]:
> On 09/15/2016 08:24 AM, Andrew Burgess wrote:
> > * Jakub Jelinek [2016-09-14 15:07:56 +0200]:
> >
> > > On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote:
> > > > In an attempt to get this patch merged (as I still think that its
> > > > correct) I've investigated, and documented a little more about how I
> > > > think things currently work. I'm sure most people reading this will
> > > > already know this, but hopefully, if my understanding is wrong someone
> > > > can point it out.
> > >
> > > I wonder if user_defined_section_attribute instead shouldn't be moved
> > > into struct function and be handled as a per-function flag then.
> >
> > That would certainly solve the problem I'm trying to address. But I
> > wonder, how is that different to looking for a section attribute on
> > the function DECL?
> I'm not sure it is significantly different. It seems like it's just an
> implementation detail. I'd err on the side of putting this into the struct
> function rather than on the DECL node simply to keep the size of DECL nodes
> from increasing. Even if you can find suitable free flag bits, those can
> likely be better used for other purposes.
I didn't add anything to the DECL, the information we need is already
there. The relevant chunk of the patch is:
@@ -2890,7 +2889,7 @@ pass_partition_blocks::gate (function *fun)
we are going to omit the reordering. */
&& optimize_function_for_speed_p (fun)
&& !DECL_COMDAT_GROUP (current_function_decl)
- && !user_defined_section_attribute);
+ && !lookup_attribute ("section", DECL_ATTRIBUTES (fun->decl)));
unsigned
I have not made any changes to add anything new to the DECL. I guess
an argument _could_ be made that looking up an attribute is too
expensive to be used in a pass::gate function (I haven't looked into
how expensive it is) but I figured that initially at least it's better
to reuse the data we already have around than to add a new flag that
duplicates something we already have.
> I'm still pondering the actual patch. It's not forgotten.
Would it help clarify things if I added some printf style tracing and
posted a trace? This might help highlight how
USER_DEFINED_SECTION_ATTRIBUTE is set in a different phase of
compilation and so can't possibly be of any use when deciding whether
or not to perform the pass or not.
I'm still keen to see this merged, so any extra leg work I can do to
help move this forward, please let me know; I'm happy to help.
Thanks,
Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On 09/15/2016 08:24 AM, Andrew Burgess wrote: * Jakub Jelinek [2016-09-14 15:07:56 +0200]: On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote: In an attempt to get this patch merged (as I still think that its correct) I've investigated, and documented a little more about how I think things currently work. I'm sure most people reading this will already know this, but hopefully, if my understanding is wrong someone can point it out. I wonder if user_defined_section_attribute instead shouldn't be moved into struct function and be handled as a per-function flag then. That would certainly solve the problem I'm trying to address. But I wonder, how is that different to looking for a section attribute on the function DECL? I'm not sure it is significantly different. It seems like it's just an implementation detail. I'd err on the side of putting this into the struct function rather than on the DECL node simply to keep the size of DECL nodes from increasing. Even if you can find suitable free flag bits, those can likely be better used for other purposes. I'm still pondering the actual patch. It's not forgotten. jeff
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
* Jakub Jelinek [2016-09-14 15:07:56 +0200]: > On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote: > > In an attempt to get this patch merged (as I still think that its > > correct) I've investigated, and documented a little more about how I > > think things currently work. I'm sure most people reading this will > > already know this, but hopefully, if my understanding is wrong someone > > can point it out. > > I wonder if user_defined_section_attribute instead shouldn't be moved > into struct function and be handled as a per-function flag then. That would certainly solve the problem I'm trying to address. But I wonder, how is that different to looking for a section attribute on the function DECL? Thanks, Andrew
Re: Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
On Wed, Sep 14, 2016 at 02:00:48PM +0100, Andrew Burgess wrote: > In an attempt to get this patch merged (as I still think that its > correct) I've investigated, and documented a little more about how I > think things currently work. I'm sure most people reading this will > already know this, but hopefully, if my understanding is wrong someone > can point it out. I wonder if user_defined_section_attribute instead shouldn't be moved into struct function and be handled as a per-function flag then. Jakub
Ping: Re: [PATCH 1/2] gcc: Remove unneeded global flag.
In an attempt to get this patch merged (as I still think that its correct) I've investigated, and documented a little more about how I think things currently work. I'm sure most people reading this will already know this, but hopefully, if my understanding is wrong someone can point it out. I've updated the patch to include a few tests, however, I have a little concern about the new test as they use '.text' and '.data' section names specifically, and I guess there's likely targets that don't use those names. I've limited the tests to GNU/Linux systems, but maybe I need to be even stricter? Anyway, the following is a description of why I think this patch is correct. The updated patch can be found at the end of the email. With a focus on the user_defined_section_attribute variable, here's how things currently work: 1. The variable user_defined_section_attribute is initialised to false at compile time. This is done in toplev.c 2. The only other place that user_defined_section_attribute is set to false is in the function rest_of_handle_final in final.c. This is part of the final compiler optimisation pass. The rest_of_handle_final function is called from the pass_final::execute method. 3. The user_defined_section_attribute is set to true in only one place, in handle_section_attribute in c-family/c-common.c. Setting user_defined_section_attribute to true always happens when the handle_section_attribute function is called, unless the target does not support named sections. 4. The handle_section_attribute function is called whenever any function or data has the section attribute attached. 5. The attribute handling function handle_section_attribute is called with the following call-stack (as an example): 1) handle_section_attribute 2) decl_attributes 3) c_decl_attributes 4) start_decl 5) c_parser_declaration 6) c_parser_external_declaration 7) c_parser_translation_unit 8) c_parse_file 9) c_common_parse_file 10) compile_file 11) do_compile 12) toplev::main 13) main The middle levels 2 to 8 could change depending on where the section attribute is encountered, but the interesting thing to take away is that calls to handle_section_attribute originate from a call to c_common_parse_file, which is called from do_compile. Right now I believe that this is always the case, this is crucial to the validity of this patch, if I've got this wrong then this patch is wrong. 6. Specifically, within the compile_file function (in toplev.c) it is the line 'lang_hooks.parse_file ();' which leads to the attribute handling functions being called. 7. The user_defined_section_attribute is checked in only one place, this is in the method pass_partition_blocks::gate, in the file bb-reorder.c. Like the reset to false in final.c (mentioned above) this checking of user_defined_section_attribute is part of a compiler optimisation pass. 8. Like the file parsing, the compiler optimisation passes originate from a call in compile_file (in toplev.c), this call occurs after the call to parse the file (obviously). The problem here that I see then is that we first parse the whole C file, handling all attributes, we then perform the optimisation passes on all functions. As user_defined_section_attribute starts as false and is set true during the C parsing whenever a section attribute is encountered, the variable will be true by the end of the parse process if there is any function or variable with a section attribute. We then perform the partition-blocks pass on the first function, which will be disabled (if user_defined_section_attribute) is true, regardless of whether it was actually that function that has a section attribute attached. We then perform the final pass on the first function at which point we reset the user_defined_section_attribute to false. When we perform the partition-blocks pass on the second function we will see user_defined_section_attribute set to false regardless of whether the function has a section attribute or not (the attribute handling functions are only called during file parse, not during the optimisation passes). There is another issue, that if a variable has a section attribute this will also cause user_defined_section_attribute to be set to true, (which makes sense based on the name 'user_defined_section_attribute') however, given what user_defined_section_attribute is actually used for, there's no reason to disable the partition-blocks pass just because some variable is assigned to a specific section. In the revised patch I disable the partition-blocks pass for a function only when the function DECL has a section attribute attached. This information is already available on the function DECL, so there's no need for us to keep a separate variable around, and so I delete user_defined_section_attribute. I've added three new tests in this latest revision of the patch. These cover is
