RFR: 8074457: Remove the non-Zero CPP Interpreter
Christian Thalinger
christian.thalinger at oracle.com
Tue Dec 22 23:35:58 UTC 2015
> On Dec 22, 2015, at 5:23 AM, Coleen Phillimore <coleen.phillimore at 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
More information about the hotspot-dev
mailing list