Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(681)

Issue 6823047: [PATCH] Fix PR 53743 and other -freorder-blocks-and-partition failures

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by tejohnson
Modified:
9 years, 6 months ago
Reviewers:
christophe.lyon, matthew.gretton-dann, stevenb.gcc, law, howarth, hubicka
CC:
davidxl, gcc-patches_gcc.gnu.org, stevenb.gcc, matthew.gretton-dann_linaro.org, christophe.lyon_linaro.org
Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/trunk/gcc/
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : Fix PR 53743 and other -freorder-blocks-and-partition failures #

Patch Set 3 : Fix PR 53743 and other -freorder-blocks-and-partition failures #

Unified diffs Side-by-side diffs Delta from patch set Stats (+438 lines, -87 lines) Patch
M basic-block.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M bb-reorder.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M bb-reorder.c View 1 2 11 chunks +123 lines, -20 lines 0 comments Download
M cfgcleanup.c View 1 2 4 chunks +19 lines, -5 lines 0 comments Download
M cfghooks.h View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M cfgrtl.c View 1 2 28 chunks +268 lines, -50 lines 0 comments Download
M function.h View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M function.c View 1 2 4 chunks +12 lines, -4 lines 0 comments Download
M hw-doloop.c View 1 2 1 chunk +1 line, -1 line 0 comments Download
M ifcvt.c View 1 2 1 chunk +3 lines, -4 lines 0 comments Download
M modulo-sched.c View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 35
tejohnson
This patch fixes three different failures I encountered while trying to use -freorder-blocks-and-partition, including the ...
11 years, 5 months ago (2012-10-30 05:20:02 UTC) #1
matthew.gretton-dann_linaro.org
On 30 October 2012 05:20, Teresa Johnson <tejohnson@google.com> wrote: > Index: cfgrtl.c > =================================================================== > ...
11 years, 5 months ago (2012-10-30 07:49:25 UTC) #2
stevenb.gcc
On Tue, Oct 30, 2012 at 8:49 AM, Matthew Gretton-Dann wrote: > On 30 October ...
11 years, 5 months ago (2012-10-30 16:28:10 UTC) #3
stevenb.gcc
On Tue, Oct 30, 2012 at 6:20 AM, Teresa Johnson wrote: > Index: bb-reorder.c > ...
11 years, 5 months ago (2012-10-30 17:48:32 UTC) #4
stevenb.gcc
On Tue, Oct 30, 2012 at 6:48 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 5 months ago (2012-10-30 18:54:09 UTC) #5
stevenb.gcc
Hello Teresa, Could you try this patch for me also? It moves bbpart outside the ...
11 years, 5 months ago (2012-10-30 21:28:38 UTC) #6
stevenb.gcc
On Tue, Oct 30, 2012 at 10:28 PM, Steven Bosscher wrote: > Hello Teresa, > ...
11 years, 5 months ago (2012-10-30 21:33:41 UTC) #7
tejohnson
On Tue, Oct 30, 2012 at 2:33 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 5 months ago (2012-10-30 21:43:37 UTC) #8
tejohnson
On Tue, Oct 30, 2012 at 2:33 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 5 months ago (2012-10-31 05:13:16 UTC) #9
stevenb.gcc
On Tue, Oct 30, 2012 at 10:43 PM, Teresa Johnson wrote: > Sure, I will ...
11 years, 5 months ago (2012-10-31 23:03:16 UTC) #10
christophe.lyon_linaro.org
> I would like to work on debugging this, but it's hard without test cases... ...
11 years, 5 months ago (2012-11-01 15:53:26 UTC) #11
tejohnson
On Wed, Oct 31, 2012 at 4:02 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 5 months ago (2012-11-01 17:19:08 UTC) #12
tejohnson
On Thu, Nov 1, 2012 at 10:19 AM, Teresa Johnson <tejohnson@google.com> wrote: > On Wed, ...
11 years, 5 months ago (2012-11-01 21:26:02 UTC) #13
tejohnson
On Tue, Oct 30, 2012 at 10:48 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 5 months ago (2012-11-01 21:35:35 UTC) #14
stevenb.gcc
On Thu, Nov 1, 2012 at 10:26 PM, Teresa Johnson wrote: > I'll do some ...
11 years, 5 months ago (2012-11-01 21:46:17 UTC) #15
tejohnson
On Thu, Nov 1, 2012 at 2:26 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Thu, ...
11 years, 4 months ago (2012-11-03 21:44:55 UTC) #16
tejohnson
On Tue, Oct 30, 2012 at 10:48 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, ...
11 years, 4 months ago (2012-11-03 22:15:14 UTC) #17
tejohnson
Ping. Teresa On Thu, Nov 15, 2012 at 12:10 PM, Teresa Johnson <tejohnson@google.com> wrote: > ...
11 years, 4 months ago (2012-11-26 15:55:10 UTC) #18
christophe.lyon_linaro.org
Hi, I have tested your patch on Spec2000 on ARM, and I can still see ...
11 years, 4 months ago (2012-11-26 16:07:00 UTC) #19
christophe.lyon_linaro.org
Hi, I have tested your patch on Spec2000 on ARM, and I can still see ...
11 years, 4 months ago (2012-11-26 16:25:10 UTC) #20
tejohnson
Are you sure you have all my changes applied? I applied the 4 patches attached ...
11 years, 4 months ago (2012-11-26 20:19:55 UTC) #21
tejohnson
Here is the patch again, updated to use the new vec implementation. Thanks, Teresa Revised ...
11 years, 4 months ago (2012-11-26 20:28:54 UTC) #22
howarth_bromo.med.uc.edu
On Mon, Nov 26, 2012 at 12:19:55PM -0800, Teresa Johnson wrote: > Are you sure ...
11 years, 4 months ago (2012-11-26 20:42:51 UTC) #23
tejohnson
Sorry, I don't know what happened there. Patch is attached. Thanks, Teresa On Mon, Nov ...
11 years, 4 months ago (2012-11-26 20:52:28 UTC) #24
christophe.lyon_linaro.org
I have updated my trunk checkout, and I can confirm that eval.c now compiles with ...
11 years, 4 months ago (2012-11-28 15:48:46 UTC) #25
tejohnson
Is this with the same target compiler and options used in PR55121? I will try ...
11 years, 4 months ago (2012-11-28 15:56:55 UTC) #26
christophe.lyon_linaro.org
Yes, I have configured GCC with: --target=arm-none-linux-gnueabi--with-cpu=cortex-a9 --with-fpu=neon --with-float=softfp Thanks, Christophe. On 28 November 2012 ...
11 years, 4 months ago (2012-11-28 17:02:59 UTC) #27
tejohnson
On Wed, Nov 28, 2012 at 7:48 AM, Christophe Lyon <christophe.lyon@linaro.org > wrote: > I ...
11 years, 3 months ago (2012-12-06 19:26:47 UTC) #28
christophe.lyon_linaro.org
Hello, Sorry for the long delay (ref http://patchwork.ozlabs.org/patch/199397/) On 6 December 2012 20:26, Teresa Johnson ...
11 years, 1 month ago (2013-01-31 14:13:16 UTC) #29
christophe.lyon_linaro.org
Hello, Sorry for the long delay (ref http://patchwork.ozlabs.org/patch/199397/) On 6 December 2012 20:26, Teresa Johnson ...
11 years, 1 month ago (2013-01-31 14:18:08 UTC) #30
tejohnson
Thanks for the confirmation that the -g issue is orthogonal. I did start to try ...
11 years, 1 month ago (2013-02-05 15:45:30 UTC) #31
tejohnson
Somehow Rietveld didn't upload the patch properly. I've attached the patch to this email instead. ...
10 years, 10 months ago (2013-05-08 05:13:52 UTC) #32
law_redhat.com
On 05/07/13 23:13, Teresa Johnson wrote: > ---------------------- > Revised patch that fixes failures encountered ...
10 years, 10 months ago (2013-05-10 04:43:02 UTC) #33
hubicka_ucw.cz
> On 05/07/13 23:13, Teresa Johnson wrote: > >---------------------- > >Revised patch that fixes failures ...
10 years, 10 months ago (2013-05-10 11:52:49 UTC) #34
tejohnson
10 years, 10 months ago (2013-05-10 14:50:27 UTC) #35
On Fri, May 10, 2013 at 4:52 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On 05/07/13 23:13, Teresa Johnson wrote:
>> >----------------------
>> >Revised patch that fixes failures encountered when enabling
>> >-freorder-blocks-and-partition, including the failure reported in PR 53743.
>> >
>> >This includes new verification code to ensure no cold blocks dominate hot
>> >blocks contributed by Steven Bosscher.
>> Seems like a reasonable verification; presumably if we have a cold
>> block dominating a hot block, then the block/edge frequencies are
>> badly broken.  Ah, just saw the comments for the other case where
>> this happens.  cold entry, but hot loop inside pushing over the
>> barrier. Arguably given a cold block in the dominator graph, all its
>> children should have their frequences scaled down to avoid that situation.
>
> Yep, also note that sanity checking anything about frequencies is really
hard.
> There are very many places in compiler that necesarilly need to invalidate
> frequencies in weird ways (at least short of rebuilding the whole profile
> from probabilities again).

Yes, as noted in the comments this was in part due to several places
where counts/frequencies were not kept in sync. Rather than try to fix
all of these, or do any scaling of frequencies, the partitioning code
now just enforces that the partitioning is sane w.r.t. the given
counts. This is done during bb partitioning. The sanity checking
routine was also useful for finding places where optimization passes
were splitting edges and causing hot blocks previously reached by both
hot and cold blocks to become dominated by cold blocks (see comments
in commit_edge_insertions in my patch), and making sure they got fixed
up.

But there is the issue of what we should do in the case of an
infrequent but non-zero entry (marked cold by maybe_hot_count_p
because its count is less than the number of training runs) that leads
to a hot loop. The code I added to the partitioning routine
(find_rarely_executed_basic_blocks_and_crossing_edges) will cause the
entry to also be placed in the hot partition. I would argue this is
the desired behavior - if the routine contains code that is very hot
for, say, 1/2 its training runs, the entry and hot loop (and
everything on the path in between) should be in the hot partition.

>
>> I can't really comment on the cfglayout and related stuff -- it was
>> added at a time when I wasn't doing much with GCC and thus I don't
>> know much about it.
>
> I think I can take a look at the cfglayout stuff. Splitting the patch would be
great.

Thanks, that would be great. I can split the patch first.

>
> Honza
>>
>> However, I like the changes to record if we've done partitioning and
>> checking those instead of flag_reorder_blocks_and_partition.  That's
>> simple enough that I'd support pulling it out as a separate patch
>> and installing immediately if that can be done so without major
>> headaches.

Ok, thanks, will do.

>>
>> I think we could do something similar with the code to verify the
>> idom of a hot block is also hot.  Though looking at the
>> implementation I wonder if it could be simplified by walking the
>> dominator tree?  I can't look at it real closely tonight though.

Looking at this code again, I agree with you. It looks like it is
going to walk cold bb's more than once and O(n^2) in the worst case. I
will fix this (there are a couple places in the patch that do a walk
to ensure that this is not violated).

>>
>> Could you pull those two logical hunks of work out into individual
>> patches.

Will do. The only complication with splitting out the dominance
checking stuff is that there are a number of changes in the patch to
ensure that we don't violate this (hot block can't be dominated by
cold block). I am not sure it makes sense or will be easy to split all
of these out. I think what I will do is try to pull the big related
chunks of them out to a separate patch (the new verification code, the
code to prevent this in the partitioning routine, and
fixup_partitions), but there are going to be a few places in the other
patch that do some fixups related to this (e.g. in rtl_split_edge)
that I would like to leave in the larger correctness patch.

Thanks,
Teresa

>>
>> jeff



--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b