RFR (M): 8000805: JMM issue: short loads are non-atomic

[Vladimir Ivanov]

Sorry for erratic examples.

I just want to clarify importance of LoadS<->LoadUS conversion. With the 
proposed fix (or simple check on out edges), there are cases when 
conversion doesn't occur anymore.

I'll remove AD changes anyway. Your solution looks much better. But I 
want do understand whether it's enough.

 >> They correspond, but indirectly.
>> There are no "(AndI (LoadUS))" rules, but "(ConvI2L (AndI (LoadUS))"
>> are present. So, if LoadS -> LoadUS transformation doesn't occur,
>> ConvI2L rules can't be applied. I addresses this by adding missing cases.
>
> I see. But in this case you can simple add second (or more) match rule:
>
>    // Load Unsigned Byte (8 bit UNsigned) with 8-bit mask into Long
> Register
>    instruct loadUB2L_immI8(iRegL dst, memory mem, immI8 mask) %{
>      match(Set dst (ConvI2L (AndI (LoadUB mem) mask)));
> +   match(Set dst (ConvI2L (AndI (LoadB mem) mask)));
Thanks for the hint! I wasn't aware about that :-) That's exactly what I 
wanted to do.

>> Oh, I see what you are suggesting now. Yes, it definitely looks better
>> than what I have now.
>>
>> What I was concerned about is slightly different. And I haven't
>> addressed it with the the current fix.
>>
>> Current solution doesn't cover the following cases:
>>      long f(short[] sa) {
>>         short s = sa[0];
>>         return (s & 0xF0) & (s & 0x0F);
>>      }
>> Before the fix, it was transformed into 2 AndI nodes which share LoadUS.
>> Now it stays LoadS till the end.
> I don't understand what the problem you have here. The expression result
> is 0 in this case.
Sorry, it was a typo - it should be "return (s & 0xF0) + (s & 0x0F);".

My concern in this particular case is that an opportunity to transform 
LoadS into LoadUS is missed. Before the fix LoadS is successfully 
transformed into LoadUS.

I see that "ConvI2L (AndI (LoadUS/LoadS))" rules aren't applicable here.
What I want to understand is whether LoadS/LoadUS conversion is 
important or not.

>> And when I talked about filtering, I was thinking about the following
>> case:
>> public void observe(Specimen s, byte[] result) {
>>     short t = s.x;
>>     result[0] = (byte) (t & 0xFF);
>>     result[1] = (byte) (t & 0xF0);
>> }
>>
>> After parsing of this method, LoadS has 3 users: LShiftI, AndI,
>> StoreB, CallStaticJava. To convert it into LoadUS I need to look at
>> LShiftI & AndI, but skip StoreB & CallStaticJava.
> Where LShiftI comes from?
It comes from i2b bytecode. In this particular case, it's illegal to 
convert LoadS to LoadUS. But only LShiftI forbids such conversion.

Best regards,
Vladimir Ivanov

> Vladimir
>
>>
>> But do these cases matter?
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 10/20/12 3:18 AM, Vladimir Kozlov wrote:
>>> Vladimir I.,
>>>
>>> Mach nodes you added do not correspond to removed ideal code - ideal
>>> code did not have ConvI2L transformations.
>>>
>>> What John suggested is to add simple check:
>>>
>>> if ((load->outcnt() == 1) && (load->unique_out() == this))
>>>
>>> to allow these transformations. I agree that such transformations could
>>> be done only after parser when the graph is complete.
>>>
>>> Why do you think you need to filter out nodes?
>>>
>>> Thanks,
>>> Vladimir K.
>>>
>>> Vladimir Ivanov wrote:
>>>> John,
>>>>
>>>> Thanks for review.
>>>>
>>>> I started with the approach you suggests, but decided that moving
>>>> these checks into AD files is a cleaner solution. The code looked more
>>>> cumbersome because of the following:
>>>>     - can't perform such transformations during parsing
>>>>     - need to filter out nodes, which aren't affected by switch
>>>> between signed/unsigned versions (like stores, calls, etc).
>>>>
>>>> If you are fine with these complications, I'll reimplement the fix w/o
>>>> touching AD files.
>>>>
>>>> I'll double-check loadS2L_immI13 rule. Just checked and it turns out
>>>> my test doesn't cover it. Thanks for catching this.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> On 10/20/12 1:32 AM, John Rose wrote:
>>>>> I have a couple of reservations about this change. I would hope that
>>>>> the AD file logic could be more factored. The simm13 operand looks
>>>>> wrong: can't it be negative?  In any case, the opt should apply up to
>>>>> 0xFFFF.
>>>>>
>>>>> I think you can (& should) keep the memnode logic if you can prove
>>>>> that all occurrences of the signed ld can be subsumed by the unsigned
>>>>> ld. This can be detected by using out edges.
>>>>>
>>>>> -- John  (on my iPhone)
>>>>>
>>>>> On Oct 19, 2012, at 11:39 AM, Vladimir Ivanov
>>>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>>>
>>>>>> http://cr.openjdk.java.net/~vlivanov/8000805/webrev.00/
>>>>>> 282 lines changed: 255 ins; 27 del; 0 mod
>>>>>>
>>>>>> Ideal transformations during parsing & IGVN may rematerialize loads
>>>>>> in order to reify useful information (like signed/unsigned load).
>>>>>> Such behavior breaks JMM - instead of a single atomic load, multiple
>>>>>> loads are performed violating consistency of loaded data.
>>>>>>
>>>>>> The fix is to disable such transformations at all, but perform
>>>>>> relevant optimizations during matching.
>>>>>>
>>>>>> It fixes only C2 part of problem. С1 has similar deficiency, but
>>>>>> it'll be addressed separately.
>>>>>>
>>>>>> Testing: failing test, test for new matching rules, JPRT, CTW (x86,
>>>>>> x64 & sparc).
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov