RFR: 8240363: Refactor Compile::Output() to its own Phase

[Erik Österlund]

Hi Vladimir,

Thank you for the review.

Thanks,
/Erik

On 2020-03-12 01:38, Vladimir Kozlov wrote:
> First, I like the idea of this refactoring.
>
> I am fine with moving _orig_pc_slot intialization to PhaseOutput where 
> it is only used. fixed_slots() should not be modified before Output() 
> is called.
>
> I have concern about indirections you are adding:
>
> C->output()->
>
> We need Claes opinion on this. On other hand it affect only code 
> generation speed.
>
> Thanks,
> Vladimir
>
> On 3/5/20 2:27 AM, Erik Österlund wrote:
>> Hi,
>>
>> The Compile::Output function comes with a lot of baggage (state and 
>> related functions), that is
>> mostly unrelated to other parts of a compilation unit. This 
>> cluttering of unrelated stuff on
>> Compile makes the code messy.This is a proposal to break out that 
>> state and embed it into a
>> separate Phase.
>>
>> This patch moves ~800 LOC away from Compile. There are more 
>> opportunities for refactoring further,
>> but my intention with this patch is to focus on things belonging to 
>> Output() in particular. I tried
>> to be as mechanical as possible, and not change logic.
>>
>> One slight difference in logic is that we used to advance the 
>> frame_slots before calling Code_Gen(),
>> and stash away the _orig_pc_slot to the value before it was advanced, 
>> way before output is called.
>> In order to have PhaseOutput live only across Output(), I changed 
>> this to advancing the frame_slots
>> as usual before Code_Gen(), and during PhaseOutput, I set the 
>> _orig_pc_slot to the previous frame_slot
>> (which now points at the sp). That bit of untangling allowed 
>> PhaseOutput to live only across the Output()
>> call, followed by code installation (which I moved into Code_Gen, to 
>> narrow down the life time of
>> PhaseOutput).
>>
>> People maintaining non-Oracle platforms might want to have a go at 
>> compiling this to see if I managed
>> to catch all code motion in the AD files.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8240363
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8240363/webrev.00/
>>
>> Testing:
>> hs-tier{1-3}
>>
>> Thanks,
>> /Erik