RFS(M): 8005031: Some cleanup in c2 to prepare for incremental inlining support

[Christian Thalinger]

On Dec 14, 2012, at 5:58 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Hi Vladimir,
> 
> Thanks for reviewing this.
> 
> 
>> Why you removed asserts in callnode.cpp?
> 
> If inlining happens after the graph has been transformed then useless projections may have gone away. Anyway, this doesn't belong in this cleanup so I moved it to the incremental inlining change. I put the assert back but it's disabled if inlining is happening after the first round of parsing.
> 
>> I think "chunk" word is not appropriate for this. Maybe "Event" or something:
>> 
>> PrintInliningChunk --> PrintInliningEvent
>> _print_inlining_chunks --> _print_inlining_list
>> 
>> Allocate the list in comp arena for use in later inlining: new(comp_arena())
> 
> Ok.

While I'm in favor of using something else than Chunk it seems that Event is not a good name choice given all the JFR events coming soon.  Maybe Message?

-- Chris

> 
>> LogCompilation output will be also affected. I remember fixing the tool for StringOpts. Could you verify that it works and fix it if it does not?
> 
> It's indeed broken but it doesn't look like a trivial fix to me. Can this be done separately? If yes, I'll file a new CR.
> Also: in the current LogCompilation late_inline entries are listed at the end:
> 
> 13    spec.benchmarks._213_javac.ScannerInputStream::read (513 bytes)
>    @ 32 java.io.InputStream::read (0 bytes)
>      type profile java/io/InputStream -> java/io/BufferedInputStream (100%)
>    @ 32 java.io.BufferedInputStream::read (49 bytes)
>    @ 95 java.io.InputStream::read (0 bytes)
>      type profile java/io/InputStream -> java/io/BufferedInputStream (100%)
>    @ 95 java.io.BufferedInputStream::read too big
>    @ 151 java.io.InputStream::read (0 bytes)
>    @ 151 java.io.InputStream::read (0 bytes)
>    @ 408 spec.benchmarks._213_javac.Environment::error never executed
>    @ 435 java.io.InputStream::read (0 bytes)
>    @ 469 java.io.InputStream::read (0 bytes)
> late inline:
>    @ 32 java.io.BufferedInputStream::read (49 bytes) (end time: 1.7170 nodes: 792 live: 538)
>      @ 12 java.io.BufferedInputStream::fill too big
>      @ 29 java.io.BufferedInputStream::getBufIfOpen (21 bytes)
>    @ 32 java.io.BufferedInputStream::read (49 bytes)
>      @ 29 java.io.BufferedInputStream::getBufIfOpen (21 bytes) (end time: 1.7180 nodes: 826 live: 569)
> 
> (strange that @29 is twice in the late inline section)
> But if there's a lot of late inlining going on, wouldn't we want to have everything together:
> 13    spec.benchmarks._213_javac.ScannerInputStream::read (513 bytes)
>    @ 32 java.io.InputStream::read (0 bytes)
>      type profile java/io/InputStream -> java/io/BufferedInputStream (100%)
>    @ 32 java.io.BufferedInputStream::read (49 bytes) (end time: 1.7170 nodes: 792 live: 538)
>      @ 12 java.io.BufferedInputStream::fill too big
>      @ 29 java.io.BufferedInputStream::getBufIfOpen (21 bytes)
>    @ 95 java.io.InputStream::read (0 bytes)
>      type profile java/io/InputStream -> java/io/BufferedInputStream (100%)
>    @ 95 java.io.BufferedInputStream::read too big
>    @ 151 java.io.InputStream::read (0 bytes)
>    @ 151 java.io.InputStream::read (0 bytes)
>    @ 408 spec.benchmarks._213_javac.Environment::error never executed
>    @ 435 java.io.InputStream::read (0 bytes)
>    @ 469 java.io.InputStream::read (0 bytes)
> 
> with maybe an annotation that marked some call site as late inline. What do you think?
> 
>> I think you should put it on igvn._worklist because PhaseIterGVN::optimize() does removal of nodes with outcount() == 0. I don't think it is safe to do it inside Ideal() call.
> 
> Ok.
> 
>> Also, file RFE (starter_task) to find all similar cases (may be add the check for input nodes in transform_old()).
> 
> Ok.
> 
> Here is a new webrev:
> 
> http://cr.openjdk.java.net/~roland/8005031/webrev.01/
> 
> Roland.