C2 verification pass shenandoah barriers

[Roman Kennke]

Hi Roland,

This is awesome! Some comments inline.

Am Dienstag, den 07.06.2016, 11:09 +0200 schrieb Roland Westrelin:
> http://cr.openjdk.java.net/~roland/shenandoah-verif/webrev.00/
> 
> The code of the verification pass is in
> ShenandoahBarrierNode::verify()/ShenandoahBarrierNode::verify_helper(
> ).
> It walks the ideal graph and whenever it encounters a Load node, it
> verifies that all paths leading to the Address input have a read or
> write barrier or don't need one. When it's a store, it checks for a
> write barrier on the Address input and if it's an oop store it looks
> for
> a read or write barrier on the ValueIn input etc. It also covers
> intrinsics implemented as a call or their own node type. If a new
> node
> type/runtime call that manipulates oops is added or if an existing
> type/runtime call is modified so some of its inputs are now oops, the
> verification code should detect the change (and fail) so the
> verification code can be updated and kept in sync with the rest of
> the
> code. The verification pass can also check that there's no useless
> barrier in the IR graph but I left that off for now as it may require
> more work. -XX:+ShenandoahVerifyOptoBarriers turns the verification
> pass
> on. It runs towards then end of the optimization passes before macro
> expansion (because it makes finding allocation easier).

Looks great!

>  I had to use a
> simple graph pattern for pointer comparison when
> ShenandoahVerifyOptoBarriers is on to avoid complex pattern matching
> in
> the verification pass.

Ok. But you need to use write barriers in this case :-) Otherwise a GC
thread could evacuate the object between the two read barriers and we
would be comparing two different copies of the same object. Note that
this problem does not occur when we put the read barriers on the false
branch (that's why the control input is absolutely necessary there).

I was thinking about using a macro node here, that simply takes the two
inputs, and gets expanded later to the complex shape. Might be useful
here.

> c1_LIRGenerator_x86.cpp has unrelated fixes I found required during
> testing.
> 
> The addnode.cpp change is related to unsafe support. See below.
> 
> The arraycopynode.cpp change disables some graph transformations that
> require barrier to be added and are illegal as is.
> 
> In graphKit.cpp, I changed the Phi input for the object null path at
> a
> barrier to be the null object. It could help the compiler optimize
> code
> better (and also makes the code of the verification pass simpler)

This was a problem child in the past. I think you noticed that it could
make the compiler crash, and added the necessary tweaks to avoid that.
I just hope that we can get the required cast_not_null() stuff
upstream? Would there be any possible disadvantage in those?


> The library_call.cpp change also adds some missing barriers and
> removes
> some that are not required in some cases.

Ok great! Those missing barriers come from recent additions to
library_call.cpp.

It's ok to commit if you change the read barriers in acmp to write
barriers.

Roman