RFR(L): 8185556: [MVT] C2 compiler support for non-flattened value type fields

Roland Westrelin rwestrel at redhat.com
Tue Sep 26 09:20:39 UTC 2017


> http://cr.openjdk.java.net/~thartmann/8185556/webrev.00/

sharedRuntime_x86_64.cpp:
 486     case T_VALUETYPE:

Why is that needed? Shouldn't we only see T_VALUETYPEPTR here?

I'm also confused by that comment:

556       // TODO change this when we use T_VALUETYPEPTR

in signature.cpp

callGenerator.cpp:

442         Node* ctl = map->control();
443         arg = ValueTypeNode::make(gvn, ctl, map->memory(), arg);
444         map->set_control(ctl);

Why the set_control()?

That comment confuses me, too:

2117   // TODO enable when using T_VALUETYPEPTR

in escape.cpp

In graphKit.cpp:

3395 Node* GraphKit::new_instance(Node* klass_node,
3396                              Node* extra_slow_test,
3397                              Node* *return_size_val,
3398                              bool deoptimize_on_exception,
3399                              ValueTypeBaseNode* value_node) {

when can we pass a ValueTypePtrNode here?

Using GraphKit with an igvn should only be safe if _delay_transform is
true, right? Should there be an assert for _delay_transform in the
GraphKit constructor?

Also, usually after the graph is constructed with GraphKit, there is a
pass of PhaseRemoveUseless to disconnect useless nodes. Your new use of
GraphKit during igvn is not followed by PhaseRemoveUseless. I wonder if
it's a problem and if some untransformed node could stay connected to
graph.

In phaseX.cpp, PhaseIterGVN::transform(), why the change?

I find the overloaded ValueTypeNode::make() calls quite confusing after
all. I think renaming them to something that's more descriptive of what
they do would be better.

Roland.


More information about the valhalla-dev mailing list