[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR: 8074457: Remove the non-Zero CPP Interpreter
From:       Christian Thalinger <christian.thalinger () oracle ! com>
Date:       2015-12-22 23:35:58
Message-ID: 288AA4F4-15C9-44FC-8845-AB7B34A0F675 () oracle ! com
[Download RAW message or body]


> On Dec 22, 2015, at 5:23 AM, Coleen Phillimore <coleen.phillimore@oracle.com> \
> wrote: 
> 
> 
> On 12/22/15 8:48 AM, Bertrand Delsart wrote:
> > Hi Coleen,
> > 
> > Overall looks good. Thanks for the cleanup.
> > 
> > A few minor issues. No showstoppers. Feel free to ignore them or handle them \
> > later. 
> > First issue in src/share/vm/interpreter/templateInterpreterGenerator.hpp, for \
> > instance for these lines : 
> > #ifdef TARGET_ARCH_aarch64
> > - # include "templateInterpreterGenerator_aarch64.hpp"
> > - #endif
> > + void bang_stack_shadow_pages(bool native_call);
> > + void generate_transcendental_entry(AbstractInterpreter::MethodKind kind, int \
> > fpargs); + #endif // TARGET_ARCH_aarch64
> > 
> > Is there a reason to remove a CPU dependent file, which could be customized by \
> > any porting group, and instead directly add port-dependent methods in a shared \
> > file ? 
> > Similar problem for AbstractInterpreter::expr_offset_in_bytes, which was defined \
> > in CPU dependent files but is now defined in abstractInterpreter.hpp, requiring a \
> > PPC and SPARC ifdef. Should we instead define new abstractInterpreter_<cpu>.hpp \
> > files, including the CPU specific part that was in interpreter_<cpu>.hpp ?
> 
> Hi Bertrand,  I was hoping for some comments on these changes in particular and I \
> should have explained my reasoning in the RFR.  I decided that a couple of ifdefs \
> in the shared code were better than the cpu dependent inclusion of header files.   \
> Mostly because the cpu dependent header files had a lot of duplicated content and \
> it was difficult to find exactly what the differences were between the cpu \
> implementations.  Also, it would make the interpreter easier to work on if the \
> interfaces were similar (you can change the same named function in all the cpu \
> files)  and this is a mechanism for doing so. 
> The example with expr_offset_in_bytes is the annoying difference in sparc, and now \
> PPC, where the TOS points one past Lesp that we filed a cleanup bug for a long time \
> ago.  It would be nice not to have this difference! 
> Lastly, I also dislike #include in the declaration of a class and so do many IDEs.

Yes, these includes are nasty.  I wish we would use inheritance for non-performance \
critical things like these.

> 
> Thank you for reviewing the code.
> Coleen
> 
> 
> > 
> > Regards,
> > 
> > Bertrand.
> > 
> > 
> > 
> > On 18/12/2015 14:49, Coleen Phillimore wrote:
> > > Summary: Remove cppInterpreter assembly files and reorganize
> > > InterpreterGenerator includes
> > > 
> > > This change is mostly removal and removing the InterpreterGenerator
> > > class and making class Interpreter a typedef.  I removed conditional
> > > includes from interpreter header files in favor of small sections with
> > > ifdefs.   Many interpreter functions are still in the wrong cpp files
> > > but I want to leave that for a follow on, to not overwhelm reviewers.
> > > This is Large but not difficult to review.  There is also more purging
> > > that can be done with Zero, but I also want to leave that as a follow on
> > > RFE.
> > > 
> > > This has been tested with RBT (most of runtime tests on x86 and sparc),
> > > JPRT and builds zero with debug/fastdebug and product.
> > > 
> > > There are changes and deletions to ppc and aarch64.   Please let me know
> > > if you want to test with this patch or leave for follow on fixes.
> > > 
> > > open webrev at http://cr.openjdk.java.net/~coleenp/8074457/
> > > bug link https://bugs.openjdk.java.net/browse/JDK-8074457
> > > 
> > > Thanks,
> > > Coleen


[prev in list] [next in list] [prev in thread] [next in thread]