Review request: Zero assembler port of HotSpot

[Gary Benson]

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?

> 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?

> 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.

> 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.

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).

> 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.

> 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?

> 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"...

> 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.

> 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?

> 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.

> 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?

> 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?

> 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.

> 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.

> 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

> 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.

Cheers,
Gary

-- 
http://gbenson.net/