[9] RFR (S): 8152773: C2: LoadNode properties aren't preserved when converting between signed/unsigned variants

[Vladimir Ivanov]

John, Vladimir, thanks for the prompt feedback!

On 3/25/16 8:57 PM, John Rose wrote:
> That looks right.
>
> 1. Look for other mentions of _depends_on_test:  I saw one in the comments above you maybe missed.
>
> (The interaction of DependsOnlyOnTest with the node hash is tricky, interaction with Pinned needs evaluation IMO.)

I'll check that.

>
> 2. This is a good moment to convert MemNode::_unaligned_access and _mismatched_access to enums,
> for the same reasons listed here:
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/cf67bfa444b1/src/share/vm/opto/memnode.hpp#l154
They are different, since they aren't set in constructor:
   void set_unaligned_access() { _unaligned_access = true; }
   bool is_unaligned_access() const { return _unaligned_access; }
   void set_mismatched_access() { _mismatched_access = true; }
   bool is_mismatched_access() const { return _mismatched_access; }

I think the motivation was to avoid massive change in all Load*Nodes. 
LoadNode::make constructs a proper flavor and then sets 
mismatched/unaligned flags.

> I think they are separate booleans for the usual reason:  They were introduced at different times.

Do you think we should pack them?

Best regards,
Vladimir Ivanov

>
> — John
>
> On Mar 25, 2016, at 10:38 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>
>> http://cr.openjdk.java.net/~vlivanov/8152773/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8152773
>>
>> AndINode::Ideal() converts signed loads to unsigned for some masks. But it constructs fresh nodes and doesn't copy LoadNode properties from the node being replaced. It means that operations marked as pinned, mismatched or unaligned will not be treated as such anymore.
>>
>> The problem was found by compiler/unsafe/UnsafeGetStableArrayElement.
>>
>> Testing: failing test, JPRT (in progress), RBT (in progress).
>>
>> Best regards,
>> Vladimir Ivanov
>