RFR(XS) 8025657: compiler/intrinsics/mathexact/ConstantTest.java fails on assert in lcm.cpp on solaris x64

[Rickard Bäckman]

Christian & Vladimir,

I added both: http://cr.openjdk.java.net/~rbackman/8025657.5/webrev/

Thanks
/R

On Oct 11, 2013, at 8:03 PM, Christian Thalinger wrote:

> 
> On Oct 11, 2013, at 10:30 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
>> Rickard,
>> 
>> First about double addExact(). I was worried about FlagsProj will have two users so that flags->unique_out() will throw assert. But it looks like IfNode::Ideal eliminated second IfNode since inputs are the same. So it may be fine and we did not see failures like this in testing.
>> 
>> About changes. I think they good. Can you add MathExactNode::iff_node() and use it instead of duplications?
> 
> Perhaps also MathExactNode::bool_node() since you need that one too.
> 
>> 
>> Thanks,
>> Vladimir
>> 
>> On 10/11/13 5:53 AM, Rickard Bäckman wrote:
>>> And someone pointed out that I don't need to have Verify.java in @compile.
>>> 
>>> Updated: http://cr.openjdk.java.net/~rbackman/8025657.4/
>>> 
>>> Thanks
>>> /R
>>> 
>>> On Oct 11, 2013, at 1:53 PM, Rickard Bäckman wrote:
>>> 
>>>> Forgot to include a new test case.
>>>> 
>>>> Updated: http://cr.openjdk.java.net/~rbackman/8025657.3/
>>>> 
>>>> /R
>>>> 
>>>> On Oct 11, 2013, at 1:04 PM, Rickard Bäckman wrote:
>>>> 
>>>>> Vladimir,
>>>>> 
>>>>> Setting the control edge for addExact and then setting the control edge for everyone who depends on the
>>>>> result from the addExact to the non throwing edge of the If that is consuming the flags from addExact makes the tests pass. No need to make the machnode pinned. Doing that only created more problems.
>>>>> 
>>>>> GVN works fine since we have an uncommon trap. So GVN will replace the AddExact, Flags and ResultProj with those of the previous one. That produces code like:
>>>>> 
>>>>> add rax, rdx
>>>>> jo ….
>>>>> add rax, rax
>>>>> 
>>>>> for java code:
>>>>> 
>>>>> result = addExact(x, y)
>>>>> result += addExact(x, y)
>>>>> 
>>>>> Here is an updated webrev: http://cr.openjdk.java.net/~rbackman/8025657.2/
>>>>> 
>>>>> Thanks
>>>>> /R
>>>>> 
>>>>> On Oct 7, 2013, at 9:31 PM, Vladimir Kozlov wrote:
>>>>> 
>>>>>> Rickard,
>>>>>> 
>>>>>> Your code is correct. What I am trying to understand is it possible to make it more general and not just hack for mathExact.
>>>>>> Also if we set control edge to addExact node (for example, during final_graph_reshape). And make its machnode pinned (in adlc InstructForm::is_pinned()). Will current gcm code work then?
>>>>>> 
>>>>>> Speaking of cotrol edge for these nodes. You allow gvn these nodes (you have hash() method). But is this correct? What happens for duplicated code when inputs are the same?
>>>>>> 
>>>>>> addExact(x,y)
>>>>>> addExact(x,y)
>>>>>> 
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> On 10/6/13 10:13 PM, Rickard Bäckman wrote:
>>>>>>> Vladimir,
>>>>>>> 
>>>>>>> both addExact and FlagsProj are in must_clone. It means they have to be close to use, but since there are 2
>>>>>>> uses of addExact this seems to be problematic.
>>>>>>> 
>>>>>>> The code in schedule_pinned_nodes takes the addExact and FlagsProj and schedules them into the same block as the jump on overflow. It seems correct to me.
>>>>>>> 
>>>>>>> Thank you
>>>>>>> /R
>>>>>>> 
>>>>>>> On Oct 4, 2013, at 8:55 PM, Vladimir Kozlov wrote:
>>>>>>> 
>>>>>>>> Rickard,
>>>>>>>> 
>>>>>>>> addExact, should in must_clone[] list since it produce flag (I think your original changes have it). With that it should be scheduled near use (see PhaseCFG::schedule_late()). The problem could be it has 2 uses.
>>>>>>>> 
>>>>>>>> FlagsProj should be scheduled near consumer of the flag (since it is also in must_clone[]). Otherwise flags state could be changes by other instructions. So moving it to addExact block is wrong. It should be opposite: addExact should at the same block as FlagsProj and conditional jump.
>>>>>>>> 
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 10/4/13 4:35 AM, Rickard Bäckman wrote:
>>>>>>>>> Hi all,
>>>>>>>>> 
>>>>>>>>> I would appreciate some reviews on this change.
>>>>>>>>> 
>>>>>>>>> On some machines when running with -XX:+DeoptimizeALot a test (test/compiler/intrinsics/mathexact/ConstantTest.java) failed with an assert.
>>>>>>>>> assert(idx >= 0) failed: index should be set
>>>>>>>>> 
>>>>>>>>> It seems that gcm would put the addExact in a different block then the jump.
>>>>>>>>> This change forces the producer of FlagsProjNode to be in the same block as
>>>>>>>>> the consumer.
>>>>>>>>> 
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8025657
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8025657.1/
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> /R
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>>> 
>