RFR(L) 8031498: Cleanup and re-factorize PhaseChaitin::build_ifg_physical() (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Jan 22 13:24:29 PST 2014

The bug id in the mail's Subject is incorrect. Should be 8031498. I fixed it.

Changes looks good.

Thanks, Vladimir

On 1/20/14 4:47 AM, Niclas Adlertz wrote:

Thank you Vladimir.

I removed the references and removed some of the line breaks. http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev04/ Kind Regards, Niclas Adlertz On 2014-01-18 00:29, Vladimir Kozlov wrote: On 1/17/14 1:53 AM, Niclas Adlertz wrote:

Vladimir,

Thank you for your comments!

Vectors are separate from floats. Please, rename the method to isfloatorvector(). Done

class Pressure. Field names start with '' to separate then from variables. Done Why you don't like to path pointer to liveout? It helped only in few places but code in interferewithlive() become complicated (IndexSet& liveout and then elements(&liveout)). I thought it looked better to send in the liveout as a reference since it was allocated on stack in buildifgphysical: IndexSet liveout(live->live(block)); But if you want me revert the change I can do that. The final consumer IndexSetIterator of liveout needs pointer. You have to add additional conversion in sources which I don't like: foo(IndexSet& liveout) { IndexSetIterator elements(&liveout); It makes code more complex to understand. Also in buildifgphysical() you have too many empty lines. Thanks, Vladimir http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev03/ Kind Regards, Niclas Adlertz On 2014-01-16 09:11, Vladimir Kozlov wrote: Niclas, Changes are good. I have few notes. Vectors are separate from floats. Please, rename the method to isfloatorvector(). class Pressure. Field names start with '' to separate then from variables. Why you don't like to path pointer to liveout? It helped only in few places but code in interferewithlive() become complicated (IndexSet& liveout and then elements(&liveout)). Thanks, Vladimir On 1/15/14 5:45 AM, Niclas Adlertz wrote: Hi again, I found a commented section that I forgot to remove in chaitin.hpp: // stores the first low-to-high transition (starting from the top of block) //uint finalhighpressureindex; Updated the webrev: http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev02/ On 2014-01-15 14:26, Niclas Adlertz wrote: Hi Rickard,

Thank you for your comments. http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev01/ Kind Regards, Niclas Adlertz On 2014-01-14 15:03, Rickard Bäckman wrote: Just a few comments on the C++ parts. I haven't verified that the logic is still the same.

1) It seems like there are a couple of functions that could be created in the new Pressure class to avoid code duplication. One example is the checkforhighpressureblock method. 2) All places that checks lrg.isfloat also check lrg.isvector. That check could be made into a boolean method on the LRG instead. The same for isUP() & masksize(). 3) To make code more generic the intpressure and floatpressure instances could be passed the INTPRESSURE and FLOATPRESSURE and keep those as instance variables. Good work. /R On 01/14, Niclas Adlertz wrote: Hi all,

This is a first step to clean up the register allocator in C2. In this change we have divided the very long method buildifgphysical() into many smaller ones, making it easier to see the steps when building the IFG (and computing the block pressure). We have also cleaned up old comments and improved old code, both by better naming and by simplifying expressions. I personally think that the easiest way to review this change is to have the old and new ifg.cpp side by side, and start from buildifgphysical(). The next step is to create a new class for these methods, isolating them and removing them from PhaseChaitin. WEBREV: http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev00/ BUG: https://bugs.openjdk.java.net/browse/JDK-8031498 Kind Regards, Niclas Adlertz

More information about the hotspot-compiler-dev mailing list