Review request: Zero assembler port of HotSpot

[Tom Rodriguez]

On Aug 25, 2009, at 4:12 AM, Gary Benson wrote:

> Hi Tom,
>
> Tom Rodriguez wrote:
>> Some generic comments first.  The normal hotspot style is for the {
>> to being always be on the same line instead of having a break in
>> between.  Many of the new files put a line break in between.
>
> Tqhis dates back from when I originally started work on all this.
> Back then the i486 and amd64 ports were separate, and the amd64
> port had a different coding style from the others.  I followed the
> amd64 convention thinking that, because it was the newest, it was
> most likely to be right (Oops!)  Would you like me to reformat it?

The original parts of amd64 were done with different formatting than  
is the standard and we've been gradually fixing it as we've gone  
along.  While we don't have a strictly specified coding standard there  
are a few constants.  2 character indentation, mixed case type names,  
lowercase with underscores for fields and methods, trailing brace on  
same line.  There are obvious counter examples like all the oop types  
start with lowercase but that the preferred style.

>
>> I don't really like the introduction of the CORE_BUILD define.  It
>> seems orthogonal to the normal way of selecting builds.  Also zero
>> is a core-like build but it's not really core so reusing core for
>> the build directory doesn't seem right.
>
> Can you elaborate on this?  From the Makefiles I got the impression
> that what "core" meant was "everything but the compiler": is that not
> correct?

Originally core was interpreter only with nothing required by the  
compilers included at all.  Core was a bit of a pain to maintain so we  
demoted it to interpreter only without the compiler itself.  Even your  
vm version changes made a distinction between Core and Zero vm.  Maybe  
it's acceptable that the name is linux_zero_core but it seems aberrant  
to me.

>
>> What's going to happen when shark is added?
>
> To build Zero without Shark you set ZERO_BUILD=true and  
> CORE_BUILD=true.
> To build Zero with Shark you set ZERO_BUILD=true and SHARK_BUILD=true.
> So CORE_BUILD=true is used only when you're building interpreter-only.

Except that we don't currently need CORE_BUILD to build core so I  
don't quote understand it's addition.

>
>> I guess I would expect the build directories to look like
>> linux_i486_zero instead of linux_zero_core which is what I think
>> you'd currently get.
>
> I originally tried doing it that way, but it was next to impossible.
> The modified Makefiles treat "zero" as the architecture of the build
> (ie a lot of the various $*ARCH variables are set to "zero").  When
> you're building with Shark you get linux_zero_shark.

But the build directory seem pretty orthogonal to everything else why  
is it hard to make this work?

> Note that this only a build-time thing; the JDK you get at the end
> will have its architecture-dependent stuff in a directory that matches
> the name of the platform (ie i386, sparc, ppc, etc).

of course.

>
>> Is there some reason you can't allow ARCH_DATA_MODEL to be specified
>> directly instead of adding ZERO_BITSPERWORD?  And couldn't the same
>> be done with ZERO_ENDIANNESS being replaced with ENDIANNESS?  I'd
>> prefer to generalize the makefiles instead of adding exceptions for
>> zero. Maybe ZERO_LIBARCH could be handled similarly, though in at
>> least one case we're inconsistent in our use of x86 vs. i386.
>
> I'm pretty sure these could be changed, the first two at any rate.

That would be better.

>
>> Can you rename method_entry_t to something like MethodEntryFunc?
>
> Sure.
>
>> I'm surprised by the amount of stubs you have.  Presumably these are
>> only because of virtuals for types you don't use?
>
> There's virtuals in there, yes, but a lot of them are simply ordinary
> methods that are referenced by other code that doesn't get used when
> you build with Zero.  I toyed with the idea of replacing all the
> calls to Unimplemented with calls to ShouldNotCallThis, just to make
> the point that they aren't used.  What do you think?

ShouldNotCallThis() seems more accurate.

>
>> Wouldn't it be better to ifndef ZERO the files that aren't needed?
>> I'm assuming it's not a huge number of files, something like
>> assembler, stubRoutines, stubGenerator and maybe some others?  Maybe
>> that gets into a rathole?
>
> I'm not sure what you mean by "ifndef ZERO the files that aren't
> needed"...

It seems like part of the reason for stubbing so much out is to deal  
with source files that you aren't actually using.  Wouldn't it be  
better to just leave them out of the build?  This could either be done  
by ifndef'ing ZERO the contents of the cpp or hpps.  The way this has  
been managed for other kinds targets is through having alternate  
includeDB_* files.  Back when core was really the interpreter only we  
were split into includeDB_core and includeDB_ci where ci was the  
compiler code support.  Maybe to properly support zero we should make  
a split.  Anyway, I'm not sure it's worth bothering with at the  
moment.  The amount of stubbing isn't huge but it could make it easy  
to break zero.

>
>> There's something unsatisfying about the structure of these changes
>> that I can't put my finger on.  Maybe it's that zero files end up
>> containing both architecture independent and architecture dependent
>> stuff. I know that's a pretty useless comment and I'm not sure what
>> to do about it.  Maybe you have some thoughts on this?  Anyway,
>> unless I can come up with something concrete doesn't worry about
>> this.
>
> Ok.
>
>> Also I only looked at the hotspot pieces so someone familiar with the
>> other parts will have to review those before they can be committed.
>
> Sure.
>
>> hotspot/make/Makefile
>>
>> Don't reuse C2_BASE_DIR:
>>
>> CORE_BASE_DIR=$(OUTPUTDIR)/$(VM_PLATFORM)_core
>
> Ok, should be simple.
>
>> hotspot/make/linux/makefiles/buildtree.make
>>
>> ZERO_ARCH_DATA_MODEL instead of ZERO_BITSPERWORD ?
>
> Or just ARCH_DATA_MODEL as you said before.

Yes.  That would be preferred.

>
>> hotspot/make/linux/makefiles/vm.make
>>
>> remove the extra def of STATIC_CXX
>
> Ah, missed that :)
>
>> hotspot/src/share/vm/runtime/icache.cpp
>>
>> Why are you ifdefing this code instead of providing an empty
>> implementation?
>
> I think it turned out neater than trying to fudge call_flush_stub to
> pass the guarantee that the first call was for the flush stub itself.
> I can try and do it that way if you like?

It seems better to handle this as a zero problem instead of ifdef'ing  
the core.  The other way to look at it is, if there's no generated  
code to be flushed why are we calling flush?

>
>> As an aside, I'm not sure it's safe to not provide an implementation
>> of this at all.  Even the installation of newly generated code
>> requires an icache flush, otherwise you might just see garbage
>> instead of the code you actually generated.  Is shark handling this
>> differently?
>
> It's safe because there is never any actual code in codebuffers in
> Zero or Shark.  In Zero everything that's executed is code compiled by
> the C++ compiler; in Shark, the cache invalidation is done by LLVM.

Ok.

>
>> hotspot/src/share/vm/runtime/jniHandles.hpp
>>
>> What's the purpose of promoting clear() to protected in  
>> JNIHandleBlock?
>> There are no subclasses of JNIHandle that I can see.  It seems like  
>> only
>> the friend declaration is needed and I don't think you need to  
>> ifdef it.
>> It seems reasonable to allow CppInterpreter access to that.
>
> What should I do, just make clear public?

Just leave in the friend but without an ifdef.

>
>> hotspot/src/share/vm/runtime/signature.hpp
>>
>> I'm ok with these change as is but it's unfortunate that there are
>> ifdefs here at all.  Any ideas how to restructure this to eliminate
>> them completely?
>
> It should be possible to remove them, but it would require the
> implementation of pass_float for i486 and sparc (which currently
> pass floats as ints).  Shall I do this?

It might be best to save it for another day instead of cluttering up  
your change.

>> hotspot/src/share/vm/utilities/vmError.cpp
>>
>> The direct inclusion of stackPrinter_zero.hpp isn't very nice and
>> the scattered ifdef to support it's use are somewhat ugly.  Is there
>> some reason the stack printing for zero can't be integrated better
>> into the existing printing logic?
>
> I'll look into this.
>
>> src/cpu/zero/vm/assembler_zero.cpp
>>
>> Why do some functions like code_fill_byte have implementations?
>> Shouldn't they be Unimplemented()?
>
> Everything with an implementation is used somewhere by something.
> For example, code_fill_byte is used by CodeBuffer::relocate_code_to
> to clear any padding left after whatever was in the codebuffer.

So what actually ends up in a code buffer with shark if not code?

>
>> src/cpu/zero/vm/assembler_zero.hpp
>>
>> Can you move the include block to the beginning of the file.
>
> Sure.  Is there a better place I can include those files though?
> I was never particularly happy with having them in that file.

Well you might move them closer to their real uses.  Maybe  
cppInterpreter_zero.hpp?  I think it depends on how broadly it really  
needs to be included.  If you can get away with forward declaring the  
types and keep the dependencies in a single cpp that would be even  
better.

>
>> src/cpu/zero/vm/bytecodeInterpreter_zero.hpp
>>
>> what's the purpose of the LOTS_OF_REGS define?  There's no reference
>> to it anywhere else.
>
> It's used in bytecodeInterpreter.cpp:
>
>  #ifdef LOTS_OF_REGS
>    register JavaThread*      THREAD = istate->thread();
>    register volatile jbyte*  BYTE_MAP_BASE = _byte_map_base;
>  #else
>    #undef THREAD
>    #define THREAD istate->thread()
>    #undef BYTE_MAP_BASE
>    #define BYTE_MAP_BASE _byte_map_base
>  #endif

Ah thanks.  I was looking at your patch and not at the rest of  
workspace.  Bad name but oh well.

>
>> hotspot/src/cpu/zero/vm/frame_zero.hpp
>>
>> Shouldn't is_deoptimizer_frame be is_deoptimized_frame?
>
> I'll change it.
>
>> hotspot/src/cpu/zero/vm/frame_zero.inline.hpp
>>
>> I think you should be calling find_blob_unsafe instead of find_blob
>> as the other frame code does.
>
> Ok.
>
> Thanks for looking into this for me.

I apologize for the wait time on this.

tom

>
> Cheers,
> Gary
>
> -- 
> http://gbenson.net/