Message ID | 20161119215926.GX5975@embecosm.com |
---|---|
State | New |
Headers | show |
On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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.
On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
* Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]: > On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
On 11/24/2016 02:40 PM, Andrew Burgess wrote: > * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]: > >> On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: >>> On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
* Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]: > On 11/24/2016 02:40 PM, Andrew Burgess wrote: > > * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]: > > > > > On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: > > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
On 11/29/2016 07:02 AM, Andrew Burgess wrote: > * Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]: > >> On 11/24/2016 02:40 PM, Andrew Burgess wrote: >>> * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]: >>> >>>> On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: >>>>> On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
* Jeff Law <law@redhat.com> [2016-11-29 10:35:50 -0700]: > On 11/29/2016 07:02 AM, Andrew Burgess wrote: > > * Jeff Law <law@redhat.com> [2016-11-28 15:08:46 -0700]: > > > > > On 11/24/2016 02:40 PM, Andrew Burgess wrote: > > > > * Christophe Lyon <christophe.lyon@linaro.org> [2016-11-21 13:47:09 +0100]: > > > > > > > > > On 20 November 2016 at 18:27, Mike Stump <mikestump@comcast.net> wrote: > > > > > > On Nov 19, 2016, at 1:59 PM, Andrew Burgess <andrew.burgess@embecosm.com> 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
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