Review request: Zero assembler port of HotSpot

Fri Aug 28 04:14:10 PDT 2009

Tom Rodriguez wrote:
> On Aug 25, 2009, at 4:12 AM, Gary Benson wrote:
> > 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.
> >
> > This 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.

Ok, I'll reformat it for my next webrev.

> > > 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'll see if I can get rid of it.

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

I don't recall (it was a long time ago!)  I'll have another go...

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

Ok, I'll change it.

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

XXX

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

Ok, I'll change it.

> The other way to look at it is, if there's no generated code to be
> flushed why are we calling flush?

There's no generated _code_, but there is data in Zero's codebuffers.
See my answer to your next question...

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

Shark's code buffers are used for two purposes:

 - HotSpot's stack walker discovers the nmethod of a compiled frames
   by finding the code buffer that contains the PC of the frame.  It
   then discovers the BCI of that frame by looking for the PC in that
   nmethod's oopmap.  In Shark, the PC points into some opaque block
   of code that LLVM generated, so it's not possible to determine the
   BCI.  To get around this, every time Shark generates an oopmap it
   inserts a byte into the code buffer and (in a roundabout way)
   supplies the address of that byte to the stack walker as the "PC"
   of that particular frame.

 - Because the code that Shark generates is an opaque blob, it's not
   possible to inline oops into the code because it's not possible
   to rewrite them.  To work around this, Shark stores "inlined" oops
   in the code buffer along with a relocation so they get rewritten
   where necessary by the standard HotSpot code.

It's not 100% necessary for _Zero_ to have stuff in code buffers --
pre-Shark versions of Zero didn't -- but doing so allows Zero and
Shark to share the same calling convention without impacting speed.

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

Cool.

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

That file (and most of the other platform-dependent headers) get
included like this:

  cppInterpreter.hpp:
    class CppInterpreter: public AbstractInterpreter {
    ...
    #include "incls/_cppInterpreter_pd.hpp.incl"
    ...
    };

so that doesn't work.  I think libffi.h is only required in a couple
of .cpp files, so I'll try and move that into there.  As for the other
two...  not sure.  I'll try and move them somewhere nicer.

Cheers,
Gary

--
http://gbenson.net/