[lworld+vector] RFR: 8307715: Integrate VectorMask/Shuffle with value/primitive classes [v6]
Xiaohong Gong
xgong at openjdk.org
Mon May 29 08:57:25 UTC 2023
On Mon, 29 May 2023 08:45:51 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:
>> This assertion fails when I run jtreg tests like Float128VectorTests.java at the start of running the tests. I didn't figure out what was wrong.
>>
>> I noticed that you added this assertion rebased on your merging patch. I'm not sure whether something is changed related to `InlineTypeNode::merge_with` which could make this assertion pass. So I think we can add this assertion back after your merging patch is in and re-see the issue if it exists. WDYT?
>
>> This assertion fails when I run jtreg tests like Float128VectorTests.java at the start of running the tests. I didn't figure out what was wrong.
>>
>> I noticed that you added this assertion rebased on your merging patch. I'm not sure whether something is changed related to `InlineTypeNode::merge_with` which could make this assertion pass. So I think we can add this assertion back after your merging patch is in and re-see the issue if it exists. WDYT?
>
> It fails because one of the input could be InlineTypeNode and other being VectorBoxNode. _merge_with_ gets called from _PhiNode::push_inline_types_through_ which should not handle VectorBoxes, we have a separate routine to push boxes across phi (_merge_through_phi_). So again it may be related to incorrect invocation of https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/cfgnode.cpp#L2596
>
> Problem here is that if we merge VectorBox with InlineTypeNode then resulting InlineTypeNode's oop (Phi) input may have VBA in one of its input, this will create a problem during [VBA elimination ](https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/vector.cpp#L628) where VBA get replaced by SafePoint without being expanded and results into a crash during subsequent loop optimization when it tries to query the type of a SafePoint node which is not a typed IR.
Yes, so we have added the changes https://github.com/openjdk/valhalla/pull/845/files#diff-e368502932198721c3ee52ace2397c5a9c6281930e83fc53a429573f6adf44fcR2570 to forbit such cases. But it seems we have to add the `VectorBox` check in a later stage (i.e. https://github.com/openjdk/valhalla/pull/845/files#diff-e368502932198721c3ee52ace2397c5a9c6281930e83fc53a429573f6adf44fcR2584), since the `VectorBox` maybe wrapped inside a cast node. I will change this back as before and have a test with adding the assertion.
-------------
PR Review Comment: https://git.openjdk.org/valhalla/pull/845#discussion_r1209090376
More information about the valhalla-dev
mailing list